Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report unsupported target properties as errors #2217

Merged
merged 1 commit into from
Feb 23, 2024

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Feb 22, 2024

Currently, if the target declaration uses a property that is not supported by the target, this is reported as a warning. This is problematic, as we and potentially users don't notice if a target property is not supported anymore. For instance, we had several uses of the flags property in the C tests although it was renamed. Also the benchmarks used properties that we removed (See lf-lang/benchmarks-lingua-franca#61).

This PR converts the warning message into an error, adjusts the unit tests, and fixes some of the C tests. I opted for simply dropping the flags property, as apparently it wasn't required and not used for a while. In particular, some tests attempted to avoid optimization, which is not needed as we compile the tests in Debug mode (without optimization) anyway.

@cmnrd cmnrd requested review from edwardalee and lhstrh February 22, 2024 14:12
@cmnrd cmnrd added the enhancement Enhancement of existing feature label Feb 22, 2024
@cmnrd cmnrd added this to the 0.7.0 milestone Feb 22, 2024
@cmnrd cmnrd changed the title Report unsupported target properties as errors. Report unsupported target properties as errors Feb 22, 2024
@cmnrd
Copy link
Collaborator Author

cmnrd commented Feb 22, 2024

@erlingrj The Zephyr test fails because it uses the old threading property. I don't understand where the LF files that the test tries to compile originate from. Could you point me to the right place?

@erlingrj
Copy link
Collaborator

Thanks. It was checking out the lf-west-template repo and trying to compile a hello world from there. I have updated it so to remove the use of the threading property: lf-lang/lf-west-template#5

@lhstrh
Copy link
Member

lhstrh commented Feb 23, 2024

There are changes in reactor-c in this PR, but I couldn't find a corresponding PR in that repository.

Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good, but why are there changes to reactor-c?

@cmnrd
Copy link
Collaborator Author

cmnrd commented Feb 23, 2024

Thanks for catching this. reactor-c got staged accidentally.

@cmnrd cmnrd force-pushed the unsupported-properties-as-errors branch from 9f82e8a to 3d63d86 Compare February 23, 2024 10:18
Currently, if the target declarationu uses a property that is not
supported by the target, this is reported as a warning. This is
problematic, as we and potentially users don't notice if a target
property is not supported anymore. For instance, we had several uses of
the `flags` property in the C tests although it was renamed. Also the
benchmarks used properties that we removed (See lf-lang/benchmarks-lingua-franca#61).

This PR converts the warning message into an error, adjusts the unit
tests, and fixes some of the C tests. I opted for simply dropping the
`flags` property, as apparently it wasn't required and not used for a while.
In particular, some tests attempted to avoid optimization, which is not
needed as we compile the tests in Debug mode (whithout optimization)
anyway.
@cmnrd cmnrd force-pushed the unsupported-properties-as-errors branch from 3d63d86 to 5b5c5e3 Compare February 23, 2024 13:37
@cmnrd
Copy link
Collaborator Author

cmnrd commented Feb 23, 2024

Fixed it.

@cmnrd cmnrd enabled auto-merge February 23, 2024 13:37
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cmnrd cmnrd added this pull request to the merge queue Feb 23, 2024
Merged via the queue into master with commit 00aa195 Feb 23, 2024
24 checks passed
@cmnrd cmnrd deleted the unsupported-properties-as-errors branch February 23, 2024 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement of existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants