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

Add more exceptions to the list #671

Merged
merged 5 commits into from
Oct 29, 2022
Merged

Add more exceptions to the list #671

merged 5 commits into from
Oct 29, 2022

Conversation

est31
Copy link
Member

@est31 est31 commented Oct 27, 2022

I would also love #667 to be merged, but this probably has to happen after this PR as this PR fixes clippy.

@est31
Copy link
Member Author

est31 commented Oct 27, 2022

r? @Mark-Simulacrum

@rustbot
Copy link

rustbot commented Oct 27, 2022

Error: This repository is not enabled to use triagebot.
Add a triagebot.toml in the root of the default branch to enable it.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

@est31
Copy link
Member Author

est31 commented Oct 28, 2022

Oh this needs to wait until after rust-lang/rust#103654 has been merged and put into nightly (edit: or I can just address the lint anyways).

@est31 est31 force-pushed the master branch 2 times, most recently from 51d6be1 to 69debfa Compare October 28, 2022 13:02
@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Oct 29, 2022

📌 Commit 02280c6 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Oct 29, 2022

⌛ Testing commit 02280c6 with merge 53db254...

@bors
Copy link
Collaborator

bors commented Oct 29, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 53db254 to master...

@bors bors merged commit 53db254 into rust-lang:master Oct 29, 2022
@RalfJung
Copy link
Member

RalfJung commented Nov 10, 2022

FWIW these are probably almost all caused by #663. Basically every crate that links libc or a proc macro shows these symptoms. Excluding some of them is fairly pointless unless we aim at eventually excluding all of them... if we have 10k crates that fail with a rate of 0.1%, excluding 10 of them makes no difference whatsoever.

@est31
Copy link
Member Author

est31 commented Nov 11, 2022

I agree that it's pointless to add crates that "won" the 0.1% lottery. And I did indeed encounter a lot of instances of #663, but the exceptions this PR is adding are different. Unlike #663, these failures were consistent: the same crates showed up in an earlier crater report too of an unrelated PR, see the commit msg of 573d5df .

I'm not 100% sure how these errors occured but they seem to be consistent. For binja-rs, I have somewhat of an idea: OUT_DIR does not change between nightly versions or different rustc builds (not sure why but local testing confirms that), and both the new and old build.rs are putting a symlink into OUT_DIR. Why the check "does the symlink already exist" doesn't work I'm not sure, maybe due to the race condition that the Path::exists docs warn you about? IDK. For the snippet of the relevant code, see the "file already exists error" section of my triage comment, but note that I was wrong with the path being erroneously calculated, upon second inspection it was calculated correctly. The glsl validation issue, which occurs in a proc macro, is less clear to me how it comes to be... but it does also fail consistently.

@est31
Copy link
Member Author

est31 commented Nov 11, 2022

FTR, except for vulkano_tri1 and vulkano_examples, I could also find the things this PR is adding in both crater runs of your PR: rust-lang/rust#102513 (comment) and rust-lang/rust#102513 (comment) . I've checked the vulkano repos, they seem to run into OOM issues (listed as build-fail), probably due to an artifact of the build environment, so likely that's why the bug is not surfacing.

Note that I sided on the side of caution when making this PR, and thus this PR only added a tiny subset of the spurious failures to the blacklist.

@RalfJung
Copy link
Member

RalfJung commented Nov 11, 2022 via email

@est31
Copy link
Member Author

est31 commented Nov 11, 2022

doesn't crater run baseline and try build in completely separate containers?

If you compare the before and after (i open them in two tabs, scroll to the same position, and then switch back and forth using ctrl + pg up / down), you see that the it downloads some dependencies both from github as well as from crates.io in one job.

Also if you check the invoked commands, it seems to set CARGO_TARGET_DIR=/opt/rustwide/target in the docker container, and also call the -v flag to mount a directory from the host into the container:

/var/lib/crater-agent-workspace/builds/worker-7/target:/opt/rustwide/target

Maybe that mapping also explains why the check is going wrong: the symlink is seen as non-existent by the check because it doesn't resolve in the host's file system.

@est31 est31 mentioned this pull request Jan 30, 2023
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 this pull request may close these issues.

5 participants