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

Fix quoting of toolchain flags passed to cmake #703

Merged
merged 1 commit into from
Jul 28, 2021

Conversation

jsharpe
Copy link
Member

@jsharpe jsharpe commented Jun 28, 2021

Previously the cmake rule was incorrectly attempting to quote the toolchain flags as they are passed into a HEREDOC in the generated bash script.
This updates the generated bash script to use local variables to pass the flags through to the CMake crosstool file generated by the HEREDOC to avoid having to escape certain characters in those strings. This approach was chosen over disabling expansion in the HEREDOC because it is still desirable to expand some environment variables within the HEREDOC to e.g. access paths for dependencies.

@jsharpe jsharpe force-pushed the crosstool_quoting branch from 18e2bc7 to e628d5f Compare June 28, 2021 16:28
@jsharpe jsharpe force-pushed the crosstool_quoting branch 9 times, most recently from 74fbae7 to 569f15d Compare July 19, 2021 07:52
@jsharpe jsharpe linked an issue Jul 19, 2021 that may be closed by this pull request
@jsharpe jsharpe force-pushed the crosstool_quoting branch 2 times, most recently from aa5dd16 to 8c6bdd2 Compare July 19, 2021 12:52
@jsharpe jsharpe changed the title Crosstool quoting Quoting Jul 19, 2021
@jsharpe jsharpe force-pushed the crosstool_quoting branch 4 times, most recently from 379f7f2 to 520dff0 Compare July 20, 2021 07:36
@jsharpe jsharpe changed the title Quoting CMake Quoting Jul 20, 2021
@jsharpe jsharpe force-pushed the crosstool_quoting branch from 520dff0 to e82ddf9 Compare July 20, 2021 07:39
@jsharpe jsharpe requested a review from UebelAndre July 20, 2021 07:51
@jsharpe jsharpe marked this pull request as ready for review July 20, 2021 07:51
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Some small comments but otherwise excited to see this fixed!

foreign_cc/private/cmake_script.bzl Outdated Show resolved Hide resolved
foreign_cc/private/cmake_script.bzl Show resolved Hide resolved
@jsharpe jsharpe changed the title CMake Quoting Fix quoting of toolchain flags passed to cmake Jul 21, 2021
@UebelAndre UebelAndre mentioned this pull request Jul 28, 2021
@jsharpe jsharpe force-pushed the crosstool_quoting branch from d0557ec to 1308557 Compare July 28, 2021 16:25
@jsharpe jsharpe requested a review from UebelAndre July 28, 2021 16:26
@jsharpe jsharpe force-pushed the crosstool_quoting branch from 1308557 to d92fb2e Compare July 28, 2021 16:27
Copy link
Collaborator

@UebelAndre UebelAndre left a comment

Choose a reason for hiding this comment

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

Awesome! Thank you!

@jsharpe jsharpe merged commit bad4ab0 into bazel-contrib:main Jul 28, 2021
@jsharpe jsharpe deleted the crosstool_quoting branch August 3, 2022 22:07
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.

Quoting incorrect in crosstool file
2 participants