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

Rustc: Simplify --sysroot code after rust#103660 #325

Merged
merged 1 commit into from
Dec 2, 2023

Conversation

xFrednet
Copy link
Member

@xFrednet xFrednet commented Dec 1, 2023

If I'm honest, I still don't fully understand when and where the --sysroot argument is required. Clippy has very few comments on the topic. Miri has a few more, but also a custom setup in their cargo-miri interface, which is probably needed for the way miri operates.

Before rust-lang/rust#103660, rustc used to use the path of the executable to get the system root, that's why Clippy and Miri had custom ways to search for the system root. When I wrote Marker's driver, I took Clippy's code for this part and refactored it for readability.

The referenced PR changed rustc's default behavior to use the path of the rustc library, which should work for Clippy, Miri and Marker. Clippy and Miri still have some minimal code, to check for an environment value. This PR mimics Clippy's behavior, with the deviation that we don't use SYSROOT, but MARKER_SYSROOT as the name of the environment value. I think this is a bit cleaner and inline with Miri, which uses MIRI_SYSROOT


Closes #48

Maybe solves #324 as well? I sadly don't have a setup to replicate the reported bug. I'm considering, if we maybe want to do a test release and ask @tigerros (the author of #324) if they can test the release binaries for us. This PR has the v0.4.2 tag as the baseline, so we can just create a PR to the hotfix/0.4 branch for that.

@Veetaha would you mind taking a look at this PR?

@xFrednet xFrednet added C-bug Category: Something isn't working D-rustc-driver Driver: Rustc Driver A-driver Area: Driver or something related to the internal working of a driver. labels Dec 1, 2023
@xFrednet xFrednet requested a review from Veetaha December 1, 2023 23:06
Copy link
Contributor

@Veetaha Veetaha left a comment

Choose a reason for hiding this comment

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

I wouldn't worry about copying clippy's behavior. The less custom setup there is without breaking anything (at least in tests) the better. It's was likely some legacy code

@xFrednet
Copy link
Member Author

xFrednet commented Dec 2, 2023

Yep agreed. Do you think we should do a patch version for this? I'm a little scared, that it might break things. But I'm guessing to 99% that it will be fine, and it might even fix the bug

@Veetaha
Copy link
Contributor

Veetaha commented Dec 2, 2023

Let's try to do a patch 0.4.3-rc. I remember there was some caveat with this with the toolchain version state during prereleases. But since the toolchain version is currently the same as in the patch there shouldn't be any problem? Anyhow, even if the released version that uses an older toolchain and we use the hotfix branch with that toolchain in it it shouldn't cause any problems I think, but I may be misremembering

@xFrednet xFrednet added this pull request to the merge queue Dec 2, 2023
@xFrednet xFrednet removed this pull request from the merge queue due to a manual request Dec 2, 2023
@xFrednet xFrednet added this pull request to the merge queue Dec 2, 2023
Merged via the queue into rust-marker:master with commit 856542e Dec 2, 2023
26 checks passed
@xFrednet xFrednet deleted the 048-correct-sysroot branch December 2, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-driver Area: Driver or something related to the internal working of a driver. C-bug Category: Something isn't working D-rustc-driver Driver: Rustc Driver
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use rustc's way to get the system root
2 participants