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.82k stars 440 forks source link

Accpet `OsStr` instead of `str` for flags #1100

Closed NobodyXu closed 3 months ago

NobodyXu commented 3 months ago

Fixed #1018

sagudev commented 3 months ago

Just note that this is a breaking change: https://github.com/servo/servo/actions/runs/9672905431/job/26686002372?pr=32616#step:12:441

NobodyXu commented 3 months ago

Oops looks like cargo-semver-check missed this one cc @obi1kenobi

678 |             build.flag(&confdefs_path.to_string_lossy());
    |                   ----  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::AsRef<std::ffi::OsStr>` is not implemented for `std::borrow::Cow<'_, str>`, which is required by `&std::borrow::Cow<'_, str>: std::convert::AsRef<std::ffi::OsStr>`
    |                   |

looks like we would have to revert the change to the interface and add a set of new interface to accept OsStr

NobodyXu commented 3 months ago

@sagudev It is unfortunate that AsRef does not work for Cow<', str>, however looking at the log, it seems that they are converting an OsStr to Cow<', str> to workaround a past limiation of our interface.

maybe the best way forward is to pass the OsStr directly?

sagudev commented 3 months ago

Yes, indeed. I just wanted to warn that this is a breaking change, although trivial to fix on our side, there might be more crates affected by this.

NobodyXu commented 3 months ago

Thank you, now that we have released 1.0.100, I'm not sure if we should yank and then revert its interface, that could break people on new releases.

On the other hand, most people are probably using old version and has yet update, so maybe we could still yank and revert?

thomcc commented 3 months ago

It's probably worth yanking rather than breaking semver if we have a way to avoid breaking semver.

thomcc commented 3 months ago

Eh, At this point we've probably missed the boat for this to be a good candidate for yanking. I may change my mind if more reports of bustage come in though.

obi1kenobi commented 3 months ago

Oops looks like cargo-semver-check missed this one cc @obi1kenobi

Yup, it can't currently check anything related to changes in types: https://predr.ag/blog/four-challenges-cargo-semver-checks-has-yet-to-tackle/#surprising-limitation-no-checking-of-types

It's tied to the next point in that blog post: a lack of sustainable project funding. I'd love to build type-aware checks, but that requires being able to pay my rent with cargo-semver-checks and we are nowhere close to that right now 😢 I have many individuals sponsoring me on GitHub, and I love them all for it. But what would really move the needle are recurring $100-$500/month company sponsorships, and I haven't been able to get any of those so far — so rent has been coming out of my savings for many months now.

For any folks negatively affected by this issue, please remember:

Companies, I'd love to put your logo on the cargo-semver-checks README, and I'll tell everyone you're sponsoring me. So dig into your marketing budget and send some love my way!