rust-lang / cc-rs

Rust library for build scripts to compile C/C++ code into a Rust library
https://docs.rs/cc
Apache License 2.0
1.79k stars 434 forks source link

Support arbitrary `OsStr`s as arguments to compiler #1018

Closed TheButlah closed 1 month ago

TheButlah commented 5 months ago

I want to pass -fdebug-prefix-map=PATH1=PATH2 to the compiler. However, to be most portable, these should be &Paths and the overall flag should be an OsString. However, I don't see a way to pass custom os strings to the builder, unless I manually recreate the logic in .compile(output) with a bunch of .to_command()s.

Could there be an easy way to pass arbitrary os strings as arguments?

NobodyXu commented 5 months ago

We could update cc::Build::flag to take OsStr, I would accept a PR for this

ChrisDenton commented 5 months ago

Note that historically (and speaking in general) UTF-8 strings have been preferred because they tend to be the common denominator between both tools and platforms. E.g. some tools only reliably take UTF-8 values and there can be cross compiling issues when using an unspecified encoding.

Which is not to pour water on the idea, but just to say OsStr can actually be a portability hazard in another way.

NobodyXu commented 5 months ago

That's a problem indeed, though accepting OsStr also seems to make sense.

E.g. some tools only reliably take UTF-8 values and there can be cross compiling issues when using an unspecified encoding.

May I ask which tools have the problem?

We should look into it and see if cc-rs still support them or if they have improved.

ChrisDenton commented 5 months ago

I don't recall a specific issue with cc-rs off the top of my head but I know of problems with passing things through to WINE on a Linux host or when it's necessary for things to be written to a text file (e.g. json). Git on Windows also deals in bytes (whereas the host is u16s) meaning anything not unicode won't be handled correctly.

NobodyXu commented 5 months ago

Let me ask in zulip to ask for more opinions on this

TheButlah commented 4 months ago

any updates?

NobodyXu commented 4 months ago

Sorry I forgot to post the update here, we concluded the discussion that it's ok to change flag to take impl AsRef<OsStr>.