(do not check in yet) Delete json error metadata for a crate if it is out-of-date.

[pnkfelix]

Delete json error metadata for a crate if it is out-of-date.

I could not figure out a good way to encode this solely in makefile rules. Even the approach here is pretty ad-hoc; it is heavily relying on the fact that all rustc crates (and thus all the json files that compiling such crates would generate on the side) have to wait for libsyntax to be built before they can be compiled, and thus it should be sound to make this check a prereq for the stamp.syntax.

(At least, that is what I hope.)

Fix #25364

[rust-highfive]

r? @alexcrichton

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

[pnkfelix]

I don't want a rubber-stamp on this fix; its a small patch, but I really do want a deep review from someone who is knowledgeable in our make system and also a Python expert.

[pnkfelix]

(oh, also, I'm think my setting ERROR_METADATA_DIR in the makefiles will not set it in the process environment, but if there is a way I can adjust things to make that happen (without setting it on each command invocation), I think that would be cool. At least, according to my understanding of the diagnostics plugin, setting that environment variable is the appropriate way to override the setting (and thus get the effect of a single point of definition for this path...)

[pnkfelix]

oh wait, this approach might be bad ... it might cause libsyntax to be rebuilt when any rustc crate changes, even when the json file is not deleted.

I ... need to think about this. I had considered trying to use an order-only dependency, but I am not sure that can fix things here. I'll leave this PR open in the hopes that a reviewer can suggest a fix, but this should not get checked in until I resolve this.

[pnkfelix]

Another approach would be to simply delete all of the json metadata files on every build. The only reason I had avoided doing that was that I figured there was an advantage to keeping the metadata files for all the crates as long as they are not stale -- e.g. it will help detect conflicts earlier when librustc changes (and has a conflict injected with librustc_typeck) while librustc_typeck stays the same.

But that cost -- of not catching conflicts earlier -- is clearly far lower cost than rebuilding libsyntax all the time.

[alexcrichton]

The strategy of just scraping a directory and checking uniqueness on each build of a crate seems quite brittle, as it appears you've found out. I think I'd personally prefer to move this kind of validation to make tidy and just purge it from the compiler altogether.

[pnkfelix]

I assume you mean ... we would still generate the side-channel .json files, and make tidy would check that they have no duplicates? (Because checking for error code duplicates on the rust source itself is obviously tough.)

[pnkfelix]

A third option, that is potentially grosser than the previous ones: Make each error diagnostics JSON file encode what source files it was built from, and then the diagnostics plugin checks if any of those files have a time stamp newer than the error diagnostics file itself; if so, the plugin leaves that diagnostics file out of the check.

It still has some error scenarios (mostly oriented around newly introduced file submodules or renamed files), but it would work pretty well.

There is a fine line indeed between grossness and elegance...

[alexcrichton]

Yeah I think generating the JSON files is fine (although if tidy can scrape the relevant files that would be even better), but the main idea is that we only check validity after the entire build completes (ensuring the files are up-to-date).

I'd personally prefer to avoid lots of infrastructure related to this as it seems like a pretty minor thing to safeguard against.

[alexcrichton]

Note that there are some compiler panics on the bots right now having to do with invalid JSON, and I suspect that make -j2 is causing corrupt JSON when two instances of the compiler write to this output file at the same time.

Perhaps it's best to remove this logic entirely and re-add it at a later date?

[alexcrichton]

I've submitted a PR to temporarily remove the validation: #25592