Skip to content

removes syntax diagnostic err code uniqueness check #25603

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

Closed

Conversation

pnkfelix
Copy link
Member

This commit removes the syntax diagnostic validation as it's been plaguing the bots for two separate reasons and needs a quick fix for now:

  1. It's possible to read the stale metadata of previous builds, causing spurious failures that can only be solved by blowing away tmp.
  2. The code does not currently handle concurrent compilations where JSON files in the metadata directory may be half-written and hence contain invalid JSON when read.

This validation should come back, but it should likely be part of a lint pass run at the very end of the build that doesn't run into consistency or concurrency problems.


This commit is a revised version of PR #25592; this approach differs in that it continues to output the JSON metadata, so that the error index page continues to work (assuming that people do not accidentally put in duplicate entries in the meantime).

@rust-highfive
Copy link
Contributor

r? @sfackler

(rust_highfive has picked a reviewer for you, use r? to override)

@michaelsproul
Copy link
Contributor

Looks good 👍

@dotdash
Copy link
Contributor

dotdash commented May 19, 2015

@bors r+

@bors
Copy link
Collaborator

bors commented May 19, 2015

📌 Commit 760daf1 has been approved by dotdash

@dotdash dotdash assigned dotdash and unassigned sfackler May 19, 2015
@dotdash
Copy link
Contributor

dotdash commented May 19, 2015

@bors p=1

unblock those bots

@michaelsproul
Copy link
Contributor

Sorry for breaking the build 😬

@alexcrichton
Copy link
Member

@bors: p=100

No worries @michaelsproul!

@bors
Copy link
Collaborator

bors commented May 19, 2015

⌛ Testing commit 760daf1 with merge a32db93...

@bors
Copy link
Collaborator

bors commented May 19, 2015

💔 Test failed - auto-mac-32-opt

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Collaborator

bors commented May 19, 2015

⌛ Testing commit 760daf1 with merge 35b01e2...

@bors
Copy link
Collaborator

bors commented May 19, 2015

💔 Test failed - auto-linux-64-x-android-t

@alexcrichton
Copy link
Member

Note that the failure there is because there was an error outputting the metadata. I think for now it may just be best to remove it and then re-add it at a later date.

@michaelsproul
Copy link
Contributor

Damn... yeah, disabling it seems like our only option. We'll also have to prevent the error index from trying to build, just in case it fails in the absence of data.

@michaelsproul
Copy link
Contributor

Sorry I can't put a PR together myself, I'm travelling interstate at the moment.

@michaelsproul
Copy link
Contributor

(Commenting out/removing the dependency that the docs target has on the error index should be sufficient)

pnkfelix added 2 commits May 21, 2015 16:37
…guing the

bots for two separate reasons and needs a quick fix for now:

1. It's possible to read the stale metadata of previous builds, causing spurious
   failures that can only be solved by blowing away `tmp`.
2. The code does not currently handle concurrent compilations where JSON files
   in the metadata directory may be half-written and hence contain invalid JSON
   when read.

This validation should come back, but it should likely be part of a lint pass
run at the very end of the build that doesn't run into consistency or
concurrency problems.

----

This commit is a revised version of PR rust-lang#25592; this approach differs
in that it continues to output the JSON metadata, so that the error
index page continues to work (assuming that people do not accidentally
put in duplicate entries in the meantime).
@pnkfelix
Copy link
Member Author

(I'm currently revising this PR to provide better feedback about the particular error that was encountered during the metadata-output step. Admittedly such feedback won't actually fix the problem immediately, so if someone would prefer to take the more severe step of removing the error index generation and all of the other code, that's fine; I just figured i would try to acquire more knowledge from the build bots in the meantime...)

@michaelsproul
Copy link
Contributor

Yeah, it would also be nice to know why the output is failing... we should probably pass the io::Error along. I'll be back at my build rig on Sunday.

@pnkfelix pnkfelix force-pushed the remove-diagnostics-uniqness-check branch from 760daf1 to 723d1df Compare May 21, 2015 22:11
@pnkfelix
Copy link
Member Author

@bors r=dotdash 723d1df p=1

@pnkfelix
Copy link
Member Author

(I'm hoping that this will either (1.) fail in the same way, but provide much better error feedback, or (2.) magically succeed this time, unwedging various builds, and then future failures will likewise provide much better error feedback...)

@bors
Copy link
Collaborator

bors commented May 21, 2015

⌛ Testing commit 723d1df with merge 7e1e838...

@bors
Copy link
Collaborator

bors commented May 21, 2015

💔 Test failed - auto-linux-64-opt

@pnkfelix
Copy link
Member Author

(I'm hoping that this will either (1.) fail in the same way, but provide much better error feedback, or (2.) magically succeed this time, unwedging various builds, and then future failures will likewise provide much better error feedback...)

Well, apparently we get the unpredicted variant (3.):

rustc: x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc
cp: x86_64-unknown-linux-gnu/stage2/lib/librustc_back
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-opt/build/src/librustc/lib.rs:173:1: 173:52 error: metadata output error os error
/home/ubuntu/src/rust-buildbot/slave/auto-linux-64-opt/build/src/librustc/lib.rs:173 __build_diagnostic_array! { librustc, DIAGNOSTICS }
                                                                                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

At this point I'm going to go with @alexcrichton 's original plan (augmented with @michaelsproul 's note) to entirely remove the error code checking and error index page generation.

@pnkfelix
Copy link
Member Author

closing in favor of PR #25706

@pnkfelix pnkfelix closed this May 22, 2015
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.

7 participants