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

Start using serde_derive in a couple places in the compiler. #56447

Closed
wants to merge 3 commits into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Dec 2, 2018

Note that this doesn't actually use serde for anything currently - I have a different branch where I started that, but I thought I'd extract this proof of concept out, to deal with build system issues.

The second commit is needed to fix this error in rustc_codegen_llvm and rustdoc:

error[E0463]: can't find crate for `serde_derive` which `rustc` depends on

This problem arises because we use different Cargo build dirs for codegen backends and rustdoc from the main rustc Cargo build dir, and without the second commit in this PR, the host deps (including serde_derive) weren't copied over to the common sysroot (shared between all builds).

Quite surprisingly, everything else seems to "just work"!
This is in part because of pre-existing logic in rustbuild to only use stage0 for build scripts when building libstd itself, which happens to be what we need to make proc macros work in all stages 🎉

cc @alexcrichton @Mark-Simulacrum @Zoxc @nikomatsakis

@rust-highfive
Copy link
Collaborator

r? @estebank

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 2, 2018
@eddyb
Copy link
Member Author

eddyb commented Dec 2, 2018

r? @alexcrichton

@Mark-Simulacrum
Copy link
Member

@bors try to check a dist build (though I don't expect it to be different)

@bors
Copy link
Contributor

bors commented Dec 2, 2018

⌛ Trying commit 7b22fcb with merge d934074...

bors added a commit that referenced this pull request Dec 2, 2018
Start using serde_derive in a couple places in the compiler.

Note that this doesn't actually use `serde` for anything currently - I have a different branch where I started that, but I thought I'd extract this proof of concept out, to deal with build system issues.

The second commit is needed to fix this error in `rustc_codegen_llvm` and `rustdoc`:
```
error[E0463]: can't find crate for `serde_derive` which `rustc` depends on
```
This problem arises because we use different Cargo build dirs for codegen backends and `rustdoc` from the main `rustc` Cargo build dir, and without the second commit in this PR, the host `deps` (including `serde_derive`) weren't copied over to the common sysroot (shared between all builds).

*Quite surprisingly*, everything else seems to "just work"!
This is in part because of pre-existing logic in `rustbuild` to only use `stage0` for build scripts when building libstd itself, which *happens to* be what we need to make proc macros work in all stages 🎉

cc @alexcrichton @Mark-Simulacrum @Zoxc @nikomatsakis
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:21b204be:start=1543791871065679542,finish=1543791926927771954,duration=55862092412
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:59:30] 
[00:59:30] running 119 tests
[00:59:33] i..ii...iii..iiii.....i...i.........i..iii.............i......i....ii...i..i.ii..............i...ii. 100/119
[00:59:33] .ii.i.....iiii.....
[00:59:33] 
[00:59:33]  finished in 3.559
[00:59:33] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:59:48] 
[00:59:48] running 118 tests
[01:00:13] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[01:00:17] ......iii.i.....ii
[01:00:17] 
[01:00:17]  finished in 29.105
[01:00:17] travis_fold:end:test_debuginfo

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Dec 3, 2018

☀️ Test successful - status-travis
State: approved= try=True

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:33bc9f16:start=1543817953872979004,finish=1543818009818555471,duration=55945576467
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
travis_time:start:test_codegen
Check compiletest suite=codegen mode=codegen (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:00] 
[00:57:00] running 119 tests
[00:57:03] i..ii...iii..iiii.....i...i.........i..iii.............i.....i.....ii...i..i.ii..............i...ii. 100/119
[00:57:04] .ii.i.....iiii.....
[00:57:04] 
[00:57:04]  finished in 3.356
[00:57:04] travis_fold:end:test_codegen

---
travis_time:start:test_debuginfo
Check compiletest suite=debuginfo mode=debuginfo-both (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:57:18] 
[00:57:18] running 118 tests
[00:57:41] .iiiii...i.....i..i...i..i.i..i.i..i.....i..i....i..........iiii.........i.i....i...i.......ii.i.i.i 100/118
[00:57:45] ......iii.i.....ii
[00:57:45] 
[00:57:45]  finished in 27.413
[00:57:45] travis_fold:end:test_debuginfo

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@eddyb
Copy link
Member Author

eddyb commented Dec 3, 2018

Looks like the last failure is caused by build_helper and cmake being copied, with the rest of the host-specific deps, but they're built by stage0/bin/rustc.

Also, in general, I feel like we should distinguish between "crates you can use from the sysroot" and "dependencies of sysroot crates", similar to how Cargo uses -Ldependency for deps.
Maybe have a directory nested in, or next to, lib/rustlib/$target/lib, for such deps.
That way, people can't just accidentally, say, use extern crate serde; and get an error from rustc_private (even if we provide a useful message, it's still suboptimal).

@alexcrichton
Copy link
Member

Hm I think I'm a bit lost here, how does this patch work? Shouldn't procedural macros always be compiled by the compiler that build the standard library in that stage? (aka not always the stage0 compiler). I think though that with our shim it's always using the stage0 compiler?

Also.. how does this work without your patches on nightly for decoupling proc_macro/rustc?

@eddyb
Copy link
Member Author

eddyb commented Dec 4, 2018

@alexcrichton So the helpfully-preexisting behavior I allude in the PR description is here:

// Almost all of the crates that we compile as part of the bootstrap may
// have a build script, including the standard library. To compile a
// build script, however, it itself needs a standard library! This
// introduces a bit of a pickle when we're compiling the standard
// library itself.
//
// To work around this we actually end up using the snapshot compiler
// (stage0) for compiling build scripts of the standard library itself.
// The stage0 compiler is guaranteed to have a libstd available for use.
//
// For other crates, however, we know that we've already got a standard
// library up and running, so we can use the normal compiler to compile
// build scripts in that situation.
//
// If LLVM support is disabled we need to use the snapshot compiler to compile
// build scripts, as the new compiler doesn't support executables.
if mode == Mode::Std || !self.config.llvm_enabled {
cargo
.env("RUSTC_SNAPSHOT", &self.initial_rustc)
.env("RUSTC_SNAPSHOT_LIBDIR", self.rustc_snapshot_libdir());
} else {
cargo
.env("RUSTC_SNAPSHOT", self.rustc(compiler))
.env("RUSTC_SNAPSHOT_LIBDIR", self.rustc_libdir(compiler));
}

That's almost literally the exact same logic I mentioned I was considering implementing on Discord, except someone already did it!
Build scripts can always use stage0/bin/rustc so they don't need it, but it still applies.

As for stage0/bin/rustc not supporting the new decoupling: we can't use stage0/bin/rustc with the locally built src/libproc_macro anyway, while src/libproc_macro is now "ABI-safe", it still needs two copies built from the same source to be compatible (we could even include a version hash in the protocol to ensure we don't accidentally mix them up).

What should happen at stage0 is that proc macros are built against (the downloaded) stage0/lib/rustlib/**/libproc_macro*, not the locally-built version, but higher stages should use the proc_macro they built (from stage1/lib/rustlib).

Note that using proc macros in the compiler has always worked, but only at stage0, my decoupling fixes stage1 onwards (but primarily only stage1).

This might be accidentally right in rustbuild, again, because the locally-built libraries of stage0 are in stage0-sysroot/lib/rustlib, while stage0 contains the downloaded versions.
But stageN/lib/rustlib where N > 0 contains locally-built-by-stageN libraries, so what we need happens to be in stageN/lib/rustlib for all N.


To be really honest, I wasn't expecting this to work either without more tweaks. But someone from the past was too clever in rustbuild and stumbled upon what we need for proc macros!

@alexcrichton
Copy link
Member

Ok this is starting to make a bit more sense I think, we'll always compile procedural macros against the stage0 proc_macro crate, and it's the proc_macro crate freshly compiled.

What I still don't think I understand though is how this works at all in stage0 right now. Currently our stage0 compiler does the old procedural macro system. This PR, however, compiles serde_derive in stage0 against the stage0 proc_macro crate previously built. That proc_macro crate is wildly incompatible with the bootstrap compiler which we're compiling with, right? How does that get hooked up correctly?

As a side note as well, does this mean that we can never change the ABI of procedural macros? If procedural macros are always compiled against the stage0 version, how do we change the signatures?

// If this was output in the `deps` dir then this is a precise file
// name (hash included) so we start tracking it.
if filename.starts_with(&target_deps_dir) {
if filename.starts_with(&host_root_dir) || filename.starts_with(&target_deps_dir) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you expand a bit on what's going on here? This feels like it shouldn't be necessary...

Copy link
Member Author

Choose a reason for hiding this comment

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

The PR description tries to explain this. It's needed for the compiler to be able to find the (host-only) proc macro dependencies of e.g. librustc, from across a Cargo build (i.e. from codegen backends).

Copy link
Member

Choose a reason for hiding this comment

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

Hm ok, I ask because I don't think that this is right. This I think is definitely not what we want when cross-compiling because it's mixing architectures of libraries in one directory. For non-cross-compiling situations this'll copy too much for things like build scripts and/or intermediate proc-macro dependencies.

Could we perhaps whitelist an explicit copy of serde_derive if necessary? Or something like that?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if cargo would tell us if artifacts are proc macros, but I think a whitelist will suffice here

Copy link
Member Author

Choose a reason for hiding this comment

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

Mixing architectures isn't a problem for rustc, and a whitelist feels hackier IMO.

I still like the idea of making the same -Ldependency= distinction Cargo makes, in the sysroot, as well (from #56447 (comment)).

In fact, we can even copy the host deps to rustlib/$host/deps instead of rustlib/$target/deps, and have the compiler look in the right place.


I should also mention I was able to work around this by adding a serde_derive dependency to librustc_codegen_llvm, but I feel like this also doesn't scale (and it builds serde_derive more than once).


I guess I'd accept the whitelisting if someone else implemented it, just to unblock @Zoxc.

@bors
Copy link
Contributor

bors commented Dec 8, 2018

☔ The latest upstream changes (presumably #56578) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

@eddyb do you perhaps want to take the strategy from #56462 to rebase and land this?

@Dylan-DPC-zz Dylan-DPC-zz added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2019
@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Jan 14, 2019

ping from triage @eddyb you need to rebase this

@TimNN
Copy link
Contributor

TimNN commented Jan 22, 2019

Ping from triage @eddyb: What is the status of this PR?

@TimNN TimNN added A-allocators Area: Custom and system allocators and removed A-allocators Area: Custom and system allocators labels Jan 29, 2019
@Dylan-DPC-zz
Copy link

ping from triage @eddyb Unfortunately we haven't heard from you on this in a while, so I'm closing the PR to keep things tidy. Don't worry though, if you'll have time again in the future please reopen this PR, we'll be happy to review it again!

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 4, 2019
@Zoxc Zoxc mentioned this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants