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

Add support for parsing *FLAGS with shlex #1181

Merged
merged 1 commit into from
Aug 14, 2024
Merged

Conversation

xbjfk
Copy link
Contributor

@xbjfk xbjfk commented Aug 12, 2024

Hello,
This PR adds support for parsing *FLAGS with the shlex crate. This effectively fixes #847, and means *FLAGS with spaces designed for meson, make and cmake will work with no extra configuration.
In order to preserve compatibility with existing setups, the behavior must be opted into with CC_PARSE_FLAGS_WITH_SHLEX, or the parse_flags_with_shlex method on the Builder.

Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thank you!

just some minor styling suggestions

src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

Thanks LGTM!

Just want @thomcc to double check this, since it involves env var, which is kind of like a new interface, and new API.

Given that we never plan on release v2 I'd like to be a little bit more careful on that.

@thomcc
Copy link
Member

thomcc commented Aug 13, 2024

I'm wary of adding a dependency since it literally always causes us big problems but I don't see why this one would so I suppose we can give it a shot.

I think I'd prefer this be optional. We probably also shouldn't mention shlex in the interfaces because we could change the implementation to another crate (or no crate).

@xbjfk
Copy link
Contributor Author

xbjfk commented Aug 13, 2024

We probably also shouldn't mention shlex in the interfaces because we could change the implementation to another crate (or no crate).

Good idea, I changed the env var to be CC_SHELL_ESCAPED_FLAGS.

I think I'd prefer this be optional

Do you mean the dependency being optional / gated behind a feature? If it might hurt the user experience a bit, but I think it would be fine since callers of cargo who set CFLAGS could use --config

@NobodyXu
Copy link
Collaborator

NobodyXu commented Aug 13, 2024

Do you mean the dependency being optional / gated behind a feature?

Given how small shlex is (no dependency, no build-script), I don't think we need to add a feature for it.

In the future we could vendor it, given that the code for splitting based on shell isn't very large, so I don't think it's necessary.

@NobodyXu
Copy link
Collaborator

Given that there's no objection, I'd merge it in

@NobodyXu NobodyXu merged commit ebe335c into rust-lang:main Aug 14, 2024
26 checks passed
@github-actions github-actions bot mentioned this pull request Aug 14, 2024
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.

Encoded CFLAGS for supporting spaces
3 participants