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 #56795

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@Zoxc
Copy link
Contributor

Zoxc commented Dec 13, 2018

This is #56447 with the build changes in #56462

r? @alexcrichton

Show resolved Hide resolved src/bootstrap/compile.rs Outdated
@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Dec 13, 2018

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:038fceaa:start=1544738046643114821,finish=1544738049604723556,duration=2961608735
$ 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
---
[00:20:14]    Compiling rustc-demangle v0.1.9
[00:20:17]    Compiling memmap v0.6.2
[00:20:17]    Compiling num_cpus v1.8.0
[00:20:22]    Compiling rustc_llvm v0.0.0 (/checkout/src/librustc_llvm)
[00:20:26] error[E0463]: can't find crate for `serde_derive` which `rustc` depends on
[00:20:26]    |
[00:20:26] 45 | #[macro_use] extern crate rustc;
[00:20:26]    |              ^^^^^^^^^^^^^^^^^^^ can't find crate
[00:20:26] 
---
[00:20:26] travis_fold:start:stage0-rustc_codegen_llvm
travis_time:start:stage0-rustc_codegen_llvm
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:20:26] Build completed unsuccessfully in 0:16:54
[00:20:26] make: *** [all] Error 1
[00:20:26] Makefile:28: recipe for target 'all' failed
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:21c35976
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Thu Dec 13 22:14:45 UTC 2018
---
travis_time:end:067d019c:start=1544739286205368245,finish=1544739286211151194,duration=5782949
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:24315e88
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:1831b318
travis_time:start:1831b318
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:07a629da
$ dmesg | grep -i kill

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)

@Zoxc Zoxc force-pushed the Zoxc:serde-poc branch from 76fdf9c to 7bd36fb Dec 13, 2018

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 13, 2018

@bors: r+

Once this is green on Travis feel free to p=1 since this is blocking other work

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 13, 2018

📌 Commit 7bd36fb has been approved by alexcrichton

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 14, 2018

@bors r=alexcrichton

@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Dec 14, 2018

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: #56351
@bors

This comment was marked as outdated.

Copy link
Contributor

bors commented Dec 14, 2018

📌 Commit 7bd36fb has been approved by alexcrichton

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 14, 2018

@bors p=1

@bors

This comment was marked as outdated.

Copy link
Contributor

bors commented Dec 14, 2018

⌛️ Testing commit 7bd36fb with merge 3102040...

bors added a commit that referenced this pull request Dec 14, 2018

Auto merge of #56795 - Zoxc:serde-poc, r=alexcrichton
Start using serde_derive in a couple places in the compiler

This is #56447 with the build changes in #56462

r? @alexcrichton
@bors

This comment was marked as resolved.

Copy link
Contributor

bors commented Dec 14, 2018

💔 Test failed - status-travis

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 14, 2018

The job dist-aarch64-linux 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:00e8c2a4:start=1544757675792605091,finish=1544757675800163007,duration=7557916
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:0d0b567d
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:342eb1ac
travis_time:start:342eb1ac
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:046b185a
$ dmesg | grep -i kill

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)

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 14, 2018

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:067c67e5:start=1544825509782484293,finish=1544825565698523232,duration=55916038939
$ 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

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)

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 14, 2018

I've changed the approach here to something that requires no build system changes. I made #[no_link] cause extern crate to no longer be reexported, so using a crate won't also require the proc macro used to build it. I should also check for pub extern crate and #[no_link], since that combination is an error. I think that doing this is a breaking change though, so I should probably make a new unstable attribute for this.

I also added a serde_derive dependency to rustc_codegen_llvm so we won't get errors building the stage1 compiler.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 14, 2018

To understand why this is a breaking change, is #[no_link] synthesized by the compiler on dependencies to procedural macros? Or is it a stable attribute we're worried about breaking?

FWIW I think it's worthwhile to get this working without a special attribute because we'll ideally want to pull in crates.io crates that have their own procedural macros, and we wouldn't necessarily want them to annotate imports.

Also I'm not sure I quite understand why rustc_codegen_llvm changes are needed, is it just to prove it out or to fix a bug?

FWIW though I do agree that not requiring procedural macros when depending on a crate is the behavior we want!

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 14, 2018

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:0db5ca40:start=1544827260137653807,finish=1544827315500349307,duration=55362695500
$ 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
---
[00:35:50]    Compiling rand_pcg v0.1.1
[00:35:50]    Compiling rand v0.6.1
[00:35:51]    Compiling parking_lot_core v0.3.0
[00:35:56]    Compiling rustdoc v0.0.0 (/checkout/src/librustdoc)
[00:35:56] error[E0463]: can't find crate for `serde_derive` which `rustc` depends on
[00:35:56]    |
[00:35:56] 34 | extern crate rustc;
[00:35:56]    | ^^^^^^^^^^^^^^^^^^^ can't find crate
[00:35:56] 
---
[00:35:56] 
[00:35:56] 
[00:35:56] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:35:56] Build completed unsuccessfully in 0:32:55
[00:35:56] Makefile:28: recipe for target 'all' failed
[00:35:56] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:0005b973
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Fri Dec 14 23:18:00 UTC 2018

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)

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 15, 2018

#[no_link] is already stable, and I think it's used to use a crate while not linking to any code. I don't think it does anything when applied to proc macros, but it can certainly appear on stable code importing proc macro crates. I'm co-opting it here to also mean no metadata linking to the crate, so it won't be needed when dependencies builds against the current crate.

In the general case for crates.io we need to build proc macros twice when cross compiling libraries which also export the macros / crates (which is the default currently). We need to build them once for the host and then again for the target. We also need to change the built libraries to refer to the target proc macros instead of the host proc macros used to compile them.

The rustc_codegen_llvm changes is needed because the bootstrap compiler doesn't have the #[no_link] change, so it requires serde_derive around. That won't cause problems for cross-compilation though as that only applies to later stages.

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 15, 2018

The job mingw-check 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:19718b0b:start=1544904461251177531,finish=1544904516443251849,duration=55192074318
$ 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=mingw-check
---
[00:05:52]     Checking parking_lot_core v0.3.0
[00:05:53]     Checking parking_lot v0.6.4
[00:05:53]     Checking tempfile v3.0.5
[00:05:54]     Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
[00:05:54] error[E0463]: can't find crate for `serde_derive` which `rustc` depends on
[00:05:54]    |
[00:05:54] 34 | extern crate rustc;
[00:05:54]    | ^^^^^^^^^^^^^^^^^^^ can't find crate
[00:05:54] 
[00:05:54] 
[00:05:54] error: aborting due to previous error
[00:05:54] 
[00:05:54] For more information about this error, try `rustc --explain E0463`.
[00:05:54] error: Could not compile `rustdoc`.
[00:05:54] 
[00:05:54] To learn more, run the command again with --verbose.
[00:05:54] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--color" "always" "--manifest-path" "/checkout/src/tools/rustdoc/Cargo.toml" "--message-format" "json"
[00:05:54] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
[00:05:54] Build completed unsuccessfully in 0:04:58
travis_time:end:292d0844:start=1544904524653343983,finish=1544904879353194240,duration=354699850257
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 1.
---
travis_time:end:1b520126:start=1544904879764218272,finish=1544904879768763118,duration=4544846
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:122805cf
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:26bd2188
travis_time:start:26bd2188
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:054ef346
$ dmesg | grep -i kill

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)

@rust-highfive

This comment was marked as resolved.

Copy link
Collaborator

rust-highfive commented Dec 15, 2018

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:0124f144:start=1544914411276497938,finish=1544914466154306241,duration=54877808303
$ 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
---
#####################################################################     97.2%
######################################################################## 100.0%
[00:01:29] extracting /checkout/obj/build/cache/2018-12-09/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:01:30]     Updating crates.io index
[00:01:37] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:37] Build completed unsuccessfully in 0:00:30
[00:01:37] make: *** [prepare] Error 1
[00:01:37] Makefile:81: recipe for target 'prepare' failed
[00:01:38] Command failed. Attempt 2/5:
[00:01:38] Command failed. Attempt 2/5:
[00:01:38] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:38] Build completed unsuccessfully in 0:00:00
[00:01:38] make: *** [prepare] Error 1
[00:01:38] Makefile:81: recipe for target 'prepare' failed
[00:01:40] Command failed. Attempt 3/5:
[00:01:40] Command failed. Attempt 3/5:
[00:01:40] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:40] Build completed unsuccessfully in 0:00:00
[00:01:40] Makefile:81: recipe for target 'prepare' failed
[00:01:40] make: *** [prepare] Error 1
[00:01:43] Command failed. Attempt 4/5:
[00:01:43] Command failed. Attempt 4/5:
[00:01:44] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:44] Build completed unsuccessfully in 0:00:00
[00:01:44] make: *** [prepare] Error 1
[00:01:44] Makefile:81: recipe for target 'prepare' failed
[00:01:48] Command failed. Attempt 5/5:
[00:01:48] Command failed. Attempt 5/5:
[00:01:48] error: the lock file /checkout/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:01:48] Build completed unsuccessfully in 0:00:00
[00:01:48] Makefile:81: recipe for target 'prepare' failed
[00:01:48] make: *** [prepare] Error 1
[00:01:48] The command has failed after 5 attempts.
---
travis_time:end:163733fc:start=1544914583783216249,finish=1544914583790336040,duration=7119791
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:1d0feaf1
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:120a27cb
travis_time:start:120a27cb
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:074ca48c
$ dmesg | grep -i kill

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)

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Dec 16, 2018

I brought back the bootstrap changes and use them only for stage0 as I was getting some problems with building two identical versions.

@Zoxc Zoxc force-pushed the Zoxc:serde-poc branch 3 times, most recently from 0028ecc to d7f121c Dec 16, 2018

@petrochenkov petrochenkov self-assigned this Dec 17, 2018

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Dec 17, 2018

Ok so just to make sure I understand what's going on here, this is adding a new attribute #[rustc_no_link] which means the proc-macro isn't actually required to be loaded later when the crate is depended on. This only affects stage1+ though so during stage0 we still copy around libraries, but the final released compiler artifact after this change will continue to not have a libdserde_derive.so in its sysroot or anywhere, right?

I personally think, though, that we still need to brainstorm a fix for this that doesn't involve a special attribute. An actionable example is that Cargo activates the derive feature of Serde which causes the serde crate to have extern crate serde_derive; with reexports of the derive macros. I'm not entirely sure how but I'm sure that'll somehow leak into rustc itself and cause libserde_derive.so to still be required, right?

Additionally, I think this strategy means that we can't pick up anything from crates.io that uses #[derive], right? Because crates on crates.io won't have the special attribute?

Would it be possible to basically not ever require the proc-macro library to be found? I'm not actually sure why we need to find it today...

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Dec 19, 2018

I'm ok with the internal #[rustc_*] attribute as a temporary solution to make things work, but would also like to see something more principled long term.

@petrochenkov petrochenkov removed their assignment Dec 19, 2018

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 24, 2018

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

@Zoxc Zoxc force-pushed the Zoxc:serde-poc branch from d7f121c to ec7bca8 Dec 29, 2018

@Zoxc

This comment has been minimized.

Copy link
Contributor

Zoxc commented Jan 4, 2019

@alexcrichton We could make #[rustc_no_link] the default if using private crates was made an hard error (it is currently a warning). That doesn't handle the case with serde exporting serde_derive macros though. That requires Cargo changes. We'd either need to build proc macros for both the host and target, or never ship binaries and teach Cargo to build libstd and compiler crates from source, in which case we only need the proc macros built for the host.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 4, 2019

I personally don't think we can take a stopgap answer here, we need a solution in mind and ideally implemented for the compiler to be able to use proc macros and for the ecosystem to also ergonomicially use proc macros as they normally would (reexports and all). We can experiment with custom attributes, but I do not believe we should land support for serde in the compiler until we solve this problem.

I don't really have an opinion about how we should solve or enable this problem myself. Any solution that doesn't break existing crates is fine by me.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 6, 2019

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

@stokhos

This comment has been minimized.

Copy link

stokhos commented Jan 14, 2019

Ping from triage @Zoxc : Have you been able to make any progress on this PR?

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Jan 22, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment