Skip to content

fix(cmake): Evaluate generator expressions in sample cargo config #91

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(cmake): Evaluate generator expressions in sample cargo config #91

wants to merge 1 commit into from

Conversation

ball-hayden
Copy link

Using GENERATE instead of WRITE expands generator expressions.

d3zd3z
d3zd3z requested changes Apr 14, 2025
View reviewed changes
Copy link
Collaborator

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

Can you explain, in the commit text, why this change is needed? This not having this result in instances of this file being incorrect? Do we have generator expression in any of the output?

Aside from that, I'm pretty sure there should be a space between the CONTENT and the double quote after it.

d3zd3z
d3zd3z reviewed Apr 15, 2025
View reviewed changes
CMakeLists.txt Outdated
@@ -129,7 +129,7 @@ ${module}.path = \"$CACHE{RUST_MODULE_DIR}/${module}\"
# symlink) in the source directory to allow various IDE tools and such to work. The build we
# invoke will override these settings, in case they are out of date. Everything set here
# should match the arguments given to the cargo build command below.
file(WRITE ${SAMPLE_CARGO_CONFIG} "
file(GENERATE OUTPUT ${SAMPLE_CARGO_CONFIG} CONTENT"
Copy link
Collaborator

Choose a reason for hiding this comment

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

There needs to be a space between CONTENT and the double quote. Otherwise:

CMake Warning (dev) at /Users/davidb/linaro/zep/modules/lang/rust/CMakeLists.txt:132:
  Syntax Warning in cmake code at column 54

  Argument not separated from preceding token by whitespace.
This warning is for project developers.  Use -Wno-dev to suppress it.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting. This worked fine for me without the space with cmake 3.31.6.
Corrected, anyway.

d3zd3z
d3zd3z requested changes Apr 15, 2025
View reviewed changes
Copy link
Collaborator

@d3zd3z d3zd3z left a comment

Choose a reason for hiding this comment

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

Please fix the quote, and maybe add a bit more text to the commit message to explain why this is needed.

Please let me know if you need me to fix this.

@ball-hayden
Copy link
Author

Sorry for the delay @d3zd3z.

I'll rebase now against main.

@ball-hayden
fix(cmake): Evaluate generator expressions in sample cargo config
e1a31f6 
Output currently includes unexpanded generator expressions, such
as `$<TARGET_PROPERTY:compiler,nostdinc_include>;` within INCLUDE_DIRS.

Using GENERATE instead of WRITE expands generator expressions
correctly.

Signed-off-by: Hayden Ball <[email protected]>
@ball-hayden ball-hayden requested a review from d3zd3z April 15, 2025 21:43
@ball-hayden
Copy link
Author

@d3zd3z CI appears to be failing here because it doesn't have permission to publish documentation to S3, which feels unrelated.

Is there anything you need from me for this to be merged, please?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d3zd3z d3zd3z d3zd3z approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ball-hayden @d3zd3z