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

Allow passing in custom RUSTFLAGS #835

Merged
merged 1 commit into from Mar 22, 2023

Conversation

opoplawski
Copy link
Contributor

Also, according to https://cmake.org/cmake/help/latest/command/add_custom_command.html the ARGS option is ignored.

@micahsnyder
Copy link
Contributor

Most of this makes sense to me, with the string(APPEND RUSTFLAGS " -g") and string(STRIP "${RUSTFLAGS}" RUSTFLAGS).
Removing ARGS also makes sense, after looking at the docs to see that the minimum cmake version we require says it is optional/ignored.

I am a little confused about one part of this change. Why do you want to remove the quotes around ${RUSTFLAGS}? It seems like that might break things if you have more than one flag.

@opoplawski
Copy link
Contributor Author

The quotes caused problems. It seems like cmake automatically quotes the arguments.

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

The quotes caused problems. It seems like cmake automatically quotes the arguments.

Very interesting. I didn't expect it to work this way, but I did some manual testing using options -D RUSTFLAGS="-g -v" and it seems good.

We may also consider adding some documentation to the top of this file explaining that you can set RUSTFLAGS cmake variable before using this module. But I don't feel strongly about it. I ran this through our automated tests a couple weeks ago and that looked good. May as well merge now.

Thanks for the PR @opoplawski

@micahsnyder micahsnyder merged commit 8147218 into Cisco-Talos:main Mar 22, 2023
19 of 24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants