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

Encoded CFLAGS for supporting spaces #847

Closed
MarijnS95 opened this issue Aug 3, 2023 · 7 comments · Fixed by #1181
Closed

Encoded CFLAGS for supporting spaces #847

MarijnS95 opened this issue Aug 3, 2023 · 7 comments · Fixed by #1181

Comments

@MarijnS95
Copy link

According to https://github.com/rust-lang/cc-rs/#external-configuration-via-environment-variables spaces nor escapes for spaces are not supported in e.g. C(XX)FLAGS. In xbuild we use CFLAGS to set a --sysroot path, and this can occasionally contain spaces on Windows machines (because the toolchain is unpacked to a user folder, and users like to have Firstname Surname as username and profile directory).

A likable solution is new CARGO_ENCODED_RUSTFLAGS-like variables, where spaces within individual args are supported by requiring the user to replace space delimiters in between separate arguments with the \x1f ASCII Unit Separator: https://doc.rust-lang.org/cargo/reference/environment-variables.html, to allow more mechanical piecing-together of these variables without messing around with nested quotes or backslash-escapes.

Referencing rust-mobile/xbuild#124, where this is one of the deciding factors to build our Android compiler support differently.

@joshtriplett
Copy link
Member

While that might be possible to address in cc-rs, it still seems likely to break in other build systems, such as invocations of make, which don't have a solution to this problem.

@MarijnS95
Copy link
Author

@joshtriplett does cc forward to other build systems or only expand these flags into argv before invoking the compiler (which should be possible and support spaces by splitting on the separator char)? Or is this to maintain feature parity with other build systems outside of cc-rs that also process CFLAGS?

@joshtriplett
Copy link
Member

I'm talking about the latter: cc-rs doesn't seem like the right place to unilaterally propagate a new standard for passing CFLAGS that no other build system will understand.

@thomcc
Copy link
Member

thomcc commented Aug 10, 2023

There admittedly are plenty of people just using cc so it's not entirely out of the question. I have mixed feelings about it though, for sure.

@xbjfk
Copy link
Contributor

xbjfk commented Aug 12, 2024

such as invocations of make, which don't have a solution to this problem.

make does have a "solution" for this problem, if you shell escape with backslashes or quotes, it works: https://asciinema.org/a/oR0RRBOmtEX1W9p4GzVQk7mIt
cmake also works with shell escaping in CFLAGS: https://asciinema.org/a/2N3CJTWNiJBMCPlagyiHr1WEL

autoconf doesn't work.

I think the ideal solution to this would be cc parsing *FLAGS with shlex, for compatibility with make and cmake, maybe behind a new environment variable CC_PARSE_FLAGS_WITH_SHLEX to not break existing setups.

This is a pretty significant issue in my opinion, given the prevalence of spaces on Windows.

@MarijnS95
Copy link
Author

Wouldn't it be better to let each variable on their own set whether shlex parsing is needed, analogous to the RUSTFLAGS vs CARGO_ENCODED_RUSTFLAGS naming? That way there's no ambiguity.

@xbjfk
Copy link
Contributor

xbjfk commented Aug 12, 2024

The extra control would be nice, but I'm not sure about the complexity.
I doubt that there are any use cases that exist currently where certain flags are needed in shlex form and others not.
Besides, in my opinion, CFLAGS and such should be the name of the var with shlex encoded flags, since that's the canonical form cmake, meson and make use.
With this method, an environment that is set up for make, meson and cmake will work just by setting CC_PARSE_FLAGS_WITH_SHLEX, rather than needing to duplicate CFLAGS and CXXFLAGS with the same content.

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 a pull request may close this issue.

4 participants