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

Create a derive macro for HashStable and allow proc macros in rustc #58013

Merged
merged 11 commits into from
Mar 8, 2019

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Jan 30, 2019

A combination of #56864 and #56795. There were complications with using serde_derive as rustc doesn't know which crate to use for the host when there is a serde_derive in the sysroot and cargo passes another on the command line built from crates.io.

r? @eddyb (for proc macro changes) @alexcrichton (for rustbuild changes) @michaelwoerister (for the macro itself)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 30, 2019
@Zoxc
Copy link
Contributor Author

Zoxc commented Jan 30, 2019

Blocked on rust-lang/cargo#6547

@michaelwoerister
Copy link
Member

The macro looks awesome. This should let us get rid of so much boilerplate. Also, the synstructure crate looks pretty sweet :)

@alexcrichton
Copy link
Member

I haven't been following this too too closely, but @Zoxc can you detail a bit more what some of the changes are to linking and such?

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 1, 2019

I'm just going to list the changes enabling proc macros.

  • We pass -Zdual-proc-macros to both cargo and rustc when cross compiling. Cargo will then build proc macros both for the host and the target and pass both to rustc for crates which depend upon them. Rustc will link to the target proc macro crate in metadata, but load the host proc macro crate and use that to execute the macros.
  • Bootstrap will now move proc macro crates compiled to the host into the sysroot at lib\rustlib\<host-triple>\lib in addition to the target crates which are moved to lib\rustlib\<target-triple>\lib
  • We require the Test target for the compiler, since that compiles libproc_macro and we need that to build proc macros used by the stage1 compiler to build stage2 compiler crates.
  • We make -Z force-unstable-if-unmarked affect proc macros too, to avoid an ICE in stability checking

@bors

This comment has been minimized.

[package]
name = "rustc_macros"
version = "0.1.0"
authors = ["The Rust Project Developers"]
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to make this a rust 2018 crate? Otherwise, somebody will come along and do that later for #58099 anyway, leading to more churn...

Copy link
Contributor

Choose a reason for hiding this comment

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

we could/should enforce this via tidy once we have all of them ported

@alexcrichton
Copy link
Member

FWIW I don't think I'll have much to add any more to this, so long as it works and bootstraps seems fine by me, I wouldn't block on me for this.

@Zoxc Zoxc force-pushed the stable-hash-macro-simple branch from c5b0065 to 441e350 Compare February 15, 2019 05:25
@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 15, 2019

We'll need to get rust-lang/cargo#6547 into beta before this can be merged.

@vext01
Copy link
Contributor

vext01 commented Feb 20, 2019

This PR is now unblocked :)

@mati865
Copy link
Contributor

mati865 commented Feb 20, 2019

@vext01 it's not that easy.
Updated Cargo has to make it's way to beta branch first, then bootstrap compiler has to updated.
After it's done this PR will be unblocked.

@Zoxc
Copy link
Contributor Author

Zoxc commented Feb 23, 2019

Ping @eddyb for review of the proc macro parts.

@bors

This comment has been minimized.

@Zoxc Zoxc force-pushed the stable-hash-macro-simple branch from 441e350 to 756241d Compare February 25, 2019 19:37
@bors

This comment has been minimized.

@Zoxc Zoxc force-pushed the stable-hash-macro-simple branch from 756241d to 5fae5f7 Compare February 26, 2019 11:44
@bors

This comment has been minimized.

@Zoxc Zoxc force-pushed the stable-hash-macro-simple branch from 5fae5f7 to 8746683 Compare March 1, 2019 00:22
@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 1, 2019

@rust-lang/compiler Do we have someone else who can review the proc macro changes in rustc here?

@rust-highfive
Copy link
Collaborator

The job i686-gnu 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.
[02:58:33] Documenting book redirect pages (i686-unknown-linux-gnu)
[02:58:34] [TIMING] TheBook { compiler: Compiler { stage: 2, host: "i686-unknown-linux-gnu" }, target: "i686-unknown-linux-gnu", name: "book" } -- 0.967
[02:58:34] Documenting stage2 std (i686-unknown-linux-gnu)
[02:58:35]     Checking core v0.0.0 (/checkout/src/libcore)
The job exceeded the maximum time limit for jobs, and has been terminated.

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 bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 7, 2019
@pietroalbini
Copy link
Member

@bors retry

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 7, 2019
@bors
Copy link
Contributor

bors commented Mar 7, 2019

⌛ Testing commit f2ef283 with merge c1187c488edaf9d9874c5ca40840552b436cf003...

@pietroalbini
Copy link
Member

@bors retry

@rust-highfive
Copy link
Collaborator

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.
Cloning into 'rust-lang/rust'...
travis_time:end:01a0ba21:start=1551943099700463655,finish=1551943105949872037,duration=6249408382
$ cd rust-lang/rust
$ git checkout -qf c1187c488edaf9d9874c5ca40840552b436cf003
fatal: reference is not a tree: c1187c488edaf9d9874c5ca40840552b436cf003
The command "git checkout -qf c1187c488edaf9d9874c5ca40840552b436cf003" failed and exited with 128 during .
Your build has been stopped.

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
Copy link
Contributor Author

Zoxc commented Mar 7, 2019

@bors p=0

@Zoxc
Copy link
Contributor Author

Zoxc commented Mar 8, 2019

@bors p=500

@bors
Copy link
Contributor

bors commented Mar 8, 2019

⌛ Testing commit f2ef283 with merge 0547ceb...

bors added a commit that referenced this pull request Mar 8, 2019
Create a derive macro for HashStable and allow proc macros in rustc

A combination of #56864 and #56795. There were complications with using `serde_derive` as rustc doesn't know which crate to use for the host when there is a serde_derive in the sysroot and cargo passes another on the command line built from crates.io.

r? @eddyb (for proc macro changes) @alexcrichton (for rustbuild changes) @michaelwoerister (for the macro itself)
@pietroalbini
Copy link
Member

@bors treeclosed- p=2

@bors
Copy link
Contributor

bors commented Mar 8, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing 0547ceb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 8, 2019
@bors bors merged commit f2ef283 into rust-lang:master Mar 8, 2019
@Zoxc Zoxc deleted the stable-hash-macro-simple branch March 8, 2019 17:57
@michaelwoerister
Copy link
Member

🎉

@@ -258,7 +260,7 @@ pub struct Context<'a> {
pub extra_filename: Option<&'a str>,
// points to either self.sess.target.target or self.sess.host, must match triple
pub target: &'a Target,
pub triple: &'a TargetTriple,
pub triple: TargetTriple,
Copy link
Member

Choose a reason for hiding this comment

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

@@ -1232,6 +1232,8 @@ options! {DebuggingOptions, DebuggingSetter, basic_debugging_options,
Use with RUST_REGION_GRAPH=help for more info"),
parse_only: bool = (false, parse_bool, [UNTRACKED],
"parse only; do not compile, assemble, or link"),
dual_proc_macros: bool = (false, parse_bool, [TRACKED],
"load proc macros for both target and host, but only link to the target"),
Copy link
Member

Choose a reason for hiding this comment

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

Sorry for being so late to see this, but can an issue be opened to track this "feature"?

Copy link
Contributor

Choose a reason for hiding this comment

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

Opened #64671

@jyn514
Copy link
Member

jyn514 commented May 1, 2021

@Zoxc what's the reason for the last commit ("make cargo a rustc tool again")? It prevents building cargo without building the whole compiler first.

@eddyb
Copy link
Member

eddyb commented May 3, 2021

@jyn514 looks like it resolves this failure #58013 (comment), and it's just a revert of e501a87 in this PR - if you look at the full diff, this PR doesn't change that part of src/bootstrap/tool.rs - sounds like you took a wrong turn during a git blame?

@jyn514
Copy link
Member

jyn514 commented May 3, 2021

Hmm ok - I was hoping to remove that dependency in #84780 (comment), I guess I should test freebsd locally.

Yes, probably wrong git blame, thanks for the pointer :)

@eddyb
Copy link
Member

eddyb commented May 3, 2021

I guess I should test freebsd locally.

I suspect this will show up on linux too, it looks like some sort of Cargo feature conflict between tools.

EDIT: keep in mind src/tools/cargo is used by everything that uses Cargo as a library (e.g. RLS or clippy):

rust/Cargo.toml

Lines 87 to 94 in c825bc4

# We want the RLS to use the version of Cargo that we've got vendored in this
# repository to ensure that the same exact version of Cargo is used by both the
# RLS and the Cargo binary itself. The RLS depends on Cargo as a git repository
# so we use a `[patch]` here to override the github repository with our local
# vendored copy.
[patch."https://github.com/rust-lang/cargo"]
cargo = { path = "src/tools/cargo" }
cargo-util = { path = "src/tools/cargo/crates/cargo-util" }

@michaelwoerister michaelwoerister removed their assignment Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.