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

Make --emit=metadata output metadata regardless of link #49289

Merged
merged 3 commits into from
Apr 14, 2018

Conversation

varkor
Copy link
Member

@varkor varkor commented Mar 23, 2018

Fixes #40109. I'm not sure whether this condition was important here or not, but I can't see why it is required (removing it doesn't cause the error the comment warns about, so I'm assuming it's safe). If this is too heavy-handed, I can special-case on OutputType::Metadata.

r? @nrc

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2018
@pietroalbini
Copy link
Member

Ping from triage @nrc! This PR needs your review.

@quxiaolong1504
Copy link

LGTM

if (sess.opts.debugging_opts.no_trans ||
!sess.opts.output_types.should_trans()) &&
crate_type == config::CrateTypeExecutable {
if sess.opts.debugging_opts.no_trans && crate_type == config::CrateTypeExecutable {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this is correct. This loop goes on to link the code units which have been generated, but if we are in !sess.opts.output_types.should_trans() mode, then there should be no code units to link. Also I think it is odd that emit=metadata and -Ztrans would have different behaviour, which is what is happening now.

However, it has been a long time since I wrote this, so I'm not really sure what should be done, sorry

Copy link
Member Author

@varkor varkor Mar 28, 2018

Choose a reason for hiding this comment

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

Would you find it acceptable just to special-case !sess.opts.output_types.should_trans() && !sess.opts.output_types.contains_key(&OutputType::Metadata) as a quick fix for now?

Copy link
Member

Choose a reason for hiding this comment

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

I'm really not sure, sorry. ping @rust-lang/compiler anybody know about this code?

@pietroalbini
Copy link
Member

Ping from triage @nrc! The author pushed new commits addressing your concerns, can you review this PR again?

@varkor
Copy link
Member Author

varkor commented Apr 9, 2018

@pietroalbini: there was a follow-up comment, but it was hidden here. Someone else from the compiler team should probably review instead.

r? @michaelwoerister

@rust-highfive rust-highfive assigned michaelwoerister and unassigned nrc Apr 9, 2018
let output_metadata = sess.opts.output_types.contains_key(&OutputType::Metadata);
let ignore_executable = sess.opts.debugging_opts.no_trans ||
!(sess.opts.output_types.should_trans() || output_metadata);
if crate_type == config::CrateTypeExecutable && ignore_executable {
Copy link
Member

Choose a reason for hiding this comment

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

For some reason I find this condition really hard to read.

Copy link
Member

Choose a reason for hiding this comment

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

Could you do the following?

let output_metadata = sess.opts.output_types.contains_key(&OutputType::Metadata);
if (sess.opts.debugging_opts.no_trans || !sess.opts.output_types.should_trans()) &&
   !output_metadata &&
   crate_type == config::CrateTypeExecutable {
    continue;
}

@michaelwoerister
Copy link
Member

@bors r+

Thanks, @varkor!

@bors
Copy link
Contributor

bors commented Apr 13, 2018

📌 Commit 7575d96 has been approved by michaelwoerister

@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 Apr 13, 2018
kennytm added a commit to kennytm/rust that referenced this pull request Apr 13, 2018
…r=michaelwoerister

Make --emit=metadata output metadata regardless of link

Fixes rust-lang#40109. I'm not sure whether this condition was important here or not, but I can't see why it is required (removing it doesn't cause the error the comment warns about, so I'm assuming it's safe). If this is too heavy-handed, I can special-case on `OutputType::Metadata`.
@bors
Copy link
Contributor

bors commented Apr 14, 2018

⌛ Testing commit 7575d96 with merge 441363cbacfdb6ddf6e905f1e6b356ba004f326e...

@bors
Copy link
Contributor

bors commented Apr 14, 2018

💔 Test failed - status-travis

@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 Apr 14, 2018
@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.
An error occurred while generating the build script.

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
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.
An error occurred while generating the build script.

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)

@kennytm
Copy link
Member

kennytm commented Apr 14, 2018

@bors retry p=6

Travis bug.

@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 Apr 14, 2018
@bors
Copy link
Contributor

bors commented Apr 14, 2018

⌛ Testing commit 7575d96 with merge cfc3465...

bors added a commit that referenced this pull request Apr 14, 2018
…erister

Make --emit=metadata output metadata regardless of link

Fixes #40109. I'm not sure whether this condition was important here or not, but I can't see why it is required (removing it doesn't cause the error the comment warns about, so I'm assuming it's safe). If this is too heavy-handed, I can special-case on `OutputType::Metadata`.

r? @nrc
@bors
Copy link
Contributor

bors commented Apr 14, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing cfc3465 to master...

@bors bors merged commit 7575d96 into rust-lang:master Apr 14, 2018
@varkor varkor deleted the emit-metadata-without-link branch April 14, 2018 16:32
Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this pull request May 17, 2018
In rust-lang#49289, rustc was changed to emit metadata for binaries, which made
it so that the librustc.rmeta file created when compiling librustc was
overwritten by the rustc-main compilation. This commit renames the
rustc-main binary to avoid this problem.

rust-lang/cargo#5524 has also been filed to
see if Cargo can learn to warn on this situation instead of leaving it
for the user to debug.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

8 participants