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

NobodyXu · 2024-06-22T15:03:06Z

Fixed #1018

It is possible that the environment variable contains non-utf8 charactes Signed-off-by: Jiahao XU <[email protected]>

… Error>` Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

…_modifiers` Signed-off-by: Jiahao XU <[email protected]>

Signed-off-by: Jiahao XU <[email protected]>

madsmtm

Definitely a good idea!

ChrisDenton

I'm ok with this but I have just one query really. Not a blocker though.

ChrisDenton · 2024-06-22T19:40:00Z

src/lib.rs

+    #[deprecated(since = "1.0.100", note = "please use `cpp_link_stdlib_path` instead")]
+    pub fn cpp_link_stdlib<'a, V: Into<Option<&'a str>>>(


Do we absolutely have to deprecate this rather than, I dunno, doing some special trait shenanigans so it works with str but OsStr also works? Maybe we do but it seems unfortunate.

No I don't think we can, Into< Option<&str>> prevents this.

Yeah, this is unfortunate. It's usually not a path, but a literal "c++" or "stdc++" so I don't know that deprecating this is worth it.

Edit: People who really care can just link the c++ stdlib as a normal library.

I will revert that

I've reverted changes to cpp_{set, link}_stdlib according to review feedback of @thomcc

thomcc

Unsure about the deprecation.

thomcc · 2024-06-23T01:38:19Z

src/lib.rs

+    #[deprecated(since = "1.0.100", note = "please use `cpp_link_stdlib_path` instead")]
+    pub fn cpp_link_stdlib<'a, V: Into<Option<&'a str>>>(


Yeah, this is unfortunate. It's usually not a path, but a literal "c++" or "stdc++" so I don't know that deprecating this is worth it.

Edit: People who really care can just link the c++ stdlib as a normal library.

src/lib.rs

Signed-off-by: Jiahao XU <[email protected]>

sagudev · 2024-06-26T05:57:52Z

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

NobodyXu · 2024-06-26T06:54:03Z

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 · 2024-06-26T08:06:39Z

@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 · 2024-06-26T08:09:59Z

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 · 2024-06-26T08:24:33Z

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 · 2024-06-26T17:05:35Z

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

thomcc · 2024-06-26T19:59:14Z

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 · 2024-06-26T21:45:31Z

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:

cargo-semver-checks can be updated to catch it in the future.
It's up to you to help make that happen. Please get your employer to sponsor me so I can build it!

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!

NobodyXu added 6 commits June 22, 2024 23:55

Update getenv to return Arc<Ostr> instead of Arc<str>

cced1bb

It is possible that the environment variable contains non-utf8 charactes Signed-off-by: Jiahao XU <[email protected]>

Update get_cpp_link_stdlib to return `Result<Option<Cow<'_, Path>>,…

4acbec4

… Error>` Signed-off-by: Jiahao XU <[email protected]>

Update cpp_{set, link}_stdlib to accept Path instead of str

55507fc

Signed-off-by: Jiahao XU <[email protected]>

Optimize prefix_for_target to ret Cow<'static, str>

2620349

Signed-off-by: Jiahao XU <[email protected]>

Accept impl AsRef<OsStr> for asm, ar and cc/cxx flags and `link_lib…

a143668

…_modifiers` Signed-off-by: Jiahao XU <[email protected]>

Improve debug logging in getenv

b452901

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu requested review from thomcc and ChrisDenton June 22, 2024 15:03

madsmtm approved these changes Jun 22, 2024

View reviewed changes

ChrisDenton reviewed Jun 22, 2024

View reviewed changes

thomcc reviewed Jun 23, 2024

View reviewed changes

NobodyXu commented Jun 23, 2024

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

NobodyXu and others added 3 commits June 23, 2024 12:03

Revert deprecation

355924b

Revert cpp_{set, link}_stdlib changes

fe98c2c

Signed-off-by: Jiahao XU <[email protected]>

Fix doc

40397fd

Signed-off-by: Jiahao XU <[email protected]>

NobodyXu merged commit a1e291a into main Jun 23, 2024
48 checks passed

NobodyXu deleted the non-utf-8 branch June 23, 2024 12:51

github-actions bot mentioned this pull request Jun 23, 2024

chore: release v1.0.100 #1102

Merged

sagudev mentioned this pull request Jun 29, 2024

build(deps): bump cc from 1.0.99 to 1.0.101 servo/servo#32616

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

NobodyXu commented Jun 22, 2024

madsmtm left a comment

ChrisDenton left a comment

ChrisDenton Jun 22, 2024

NobodyXu Jun 23, 2024

thomcc Jun 23, 2024 •

edited

Loading

NobodyXu Jun 23, 2024

NobodyXu Jun 23, 2024

NobodyXu Jun 23, 2024

thomcc left a comment

thomcc Jun 23, 2024 •

edited

Loading

sagudev commented Jun 26, 2024

NobodyXu commented Jun 26, 2024

NobodyXu commented Jun 26, 2024

sagudev commented Jun 26, 2024

NobodyXu commented Jun 26, 2024

thomcc commented Jun 26, 2024

thomcc commented Jun 26, 2024

obi1kenobi commented Jun 26, 2024 •

edited

Loading

		#[deprecated(since = "1.0.100", note = "please use `cpp_link_stdlib_path` instead")]
		pub fn cpp_link_stdlib<'a, V: Into<Option<&'a str>>>(

Accpet OsStr instead of str for flags #1100

Accpet OsStr instead of str for flags #1100

Conversation

NobodyXu commented Jun 22, 2024

madsmtm left a comment

Choose a reason for hiding this comment

ChrisDenton left a comment

Choose a reason for hiding this comment

ChrisDenton Jun 22, 2024

Choose a reason for hiding this comment

NobodyXu Jun 23, 2024

Choose a reason for hiding this comment

thomcc Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

NobodyXu Jun 23, 2024

Choose a reason for hiding this comment

NobodyXu Jun 23, 2024

Choose a reason for hiding this comment

NobodyXu Jun 23, 2024

Choose a reason for hiding this comment

thomcc left a comment

Choose a reason for hiding this comment

thomcc Jun 23, 2024 • edited Loading

Choose a reason for hiding this comment

sagudev commented Jun 26, 2024

NobodyXu commented Jun 26, 2024

NobodyXu commented Jun 26, 2024

sagudev commented Jun 26, 2024

NobodyXu commented Jun 26, 2024

thomcc commented Jun 26, 2024

thomcc commented Jun 26, 2024

obi1kenobi commented Jun 26, 2024 • edited Loading

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

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

thomcc Jun 23, 2024 •

edited

Loading

thomcc Jun 23, 2024 •

edited

Loading

obi1kenobi commented Jun 26, 2024 •

edited

Loading