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

Correctly get source for metatdata-only crate type #40542

Merged
merged 2 commits into from
Mar 23, 2017

Conversation

abonander
Copy link
Contributor

@abonander abonander commented Mar 15, 2017

Closes #40535

However, I'm not sure how to approach writing a regression test since I'm still working on a reduced test case from the code that caused the ICE in the first place. It's not enough to have an unknown extern crate in a metadata crate, it depends on a few extra arguments but I'm not sure which yet.

Also replaced the unwrap() with a more informative expect().

r? @jseyfried

@abonander abonander changed the title Correctly get source for metadata crate type Correctly get source for metatdata-only crate type Mar 15, 2017
@arielb1 arielb1 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 16, 2017
@arielb1
Copy link
Contributor

arielb1 commented Mar 16, 2017

Test please.

@nikomatsakis
Copy link
Contributor

Marking as beta-nominated since this affects the current beta.

@jseyfried jseyfried added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Mar 16, 2017
@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Mar 16, 2017
@nikomatsakis
Copy link
Contributor

Marking as beta-accepted, once it is r+'d of course (and yes, with test plz).

@abonander
Copy link
Contributor Author

abonander commented Mar 16, 2017

Okay, so I've got a minimal case here, involving three crates, foo, bar, baz:

  • Crate foo depends on bar but declares both extern crate bar and extern crate baz
  • Crate bar depends on baz and declares extern crate baz.
  • cargo check on foo produces the ICE; cargo build or any missing extern crate just produces the "unknown crate baz" error.

I know about auxiliary builds but how do I force the --emit=metadata flag and have one auxiliary crate depend on another?

@abonander
Copy link
Contributor Author

I've posted the reproduction at https://github.com/abonander/rust-issue-40535

@arielb1
Copy link
Contributor

arielb1 commented Mar 16, 2017

You can create a run-make test (see src/test/run-make).

@abonander
Copy link
Contributor Author

@arielb1 Can that include Cargo invocations or do I have to write out the commands manually?

@abonander
Copy link
Contributor Author

Eh I can just copy the output of cargo check -v.

@abonander
Copy link
Contributor Author

Regression test pushed, it detects the error and confirms the fix correctly (fails on ICE, succeeds otherwise)

Copy link
Contributor

@jseyfried jseyfried left a comment

Choose a reason for hiding this comment

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

Thanks! r=me modulo comment

$(RUSTC) bar.rs --emit=metadata --extern baz=libbaz.rmeta
$(RUSTC) foo.rs --emit=metadata -L dependency=. --extern bar=libbar.rmeta 2>&1 | \
grep -vq "unexpectedly panicked"
# ^ Succeeds if it doesn't find the ICE message
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add trailing newline (all new files)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really gotta figure out how to get IDEA to add those automatically, it's so easy to forget.

@abonander
Copy link
Contributor Author

@jseyfried 👍

@jseyfried
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Mar 16, 2017

📌 Commit e670335 has been approved by jseyfried

@abonander
Copy link
Contributor Author

error: could not write output to baz.crate.metadata.o: Read-only file system

Not sure how to fix this.

@jseyfried
Copy link
Contributor

@abonander The run-make tests can only write in $(TMPDIR).
Note that $(RUSTC) outputs to $(TMPDIR) by default.

@abonander
Copy link
Contributor Author

Why is it not writing there now then? Weird.

@jseyfried
Copy link
Contributor

jseyfried commented Mar 17, 2017

Not sure -- looks like --emit=metadata isn't respecting --out-dir.
cc @nrc

@arielb1
Copy link
Contributor

arielb1 commented Mar 17, 2017

@bors r-

@abonander
Copy link
Contributor Author

I'm testing explicitly adding --out-dir=$(TMPDIR) to see if it fixes it, I'll squash it down if it works.

$(RUSTC) baz.rs --emit=metadata
$(RUSTC) bar.rs --emit=metadata --extern baz=libbaz.rmeta
$(RUSTC) foo.rs --emit=metadata -L dependency=. --extern bar=libbar.rmeta 2>&1 | \
$(RUSTC) baz.rs --emit=metadata --out-dir=$(TMPDIR)
Copy link
Contributor

Choose a reason for hiding this comment

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

--out-dir=$(TMPDIR) is already included in $(RUSTC), would be very strange if adding it again here made a difference...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may need to be after --emit=metadata, because that's how Cargo writes the command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc #40623

@abonander
Copy link
Contributor Author

@jseyfried Tests passed this time. I guess --emit=metadata overrides any --out-dir that appears before it, for some reason.

@alexcrichton
Copy link
Member

@bors: r=jseyfried

What weird bug!

@bors
Copy link
Contributor

bors commented Mar 18, 2017

📌 Commit 3cbdb8f has been approved by jseyfried

@abonander
Copy link
Contributor Author

@bors r-

Let me take care of that "(squash me!) ..." commit first, unless you don't care about that showing up in the commit history (which would be pretty funny to come across later, I guess).

@abonander
Copy link
Contributor Author

Should I leave a FIXME in the Makefile pointing to the issue I opened about --emit and --out-dir?

all:
$(RUSTC) baz.rs --emit=metadata --out-dir=$(TMPDIR)
$(RUSTC) bar.rs --emit=metadata --extern baz=$(TMPDIR)/libbaz.rmeta --out-dir=$(TMPDIR)
$(RUSTC) foo.rs --emit=metadata -L dependency=. --extern bar=$(TMPDIR)/libbar.rmeta --out-dir=$(TMPDIR) 2>&1 | \
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think -L dependency=. has any effect here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it seemed to before, but now it's fine.

@jseyfried
Copy link
Contributor

@abonander

Should I leave a FIXME in the Makefile pointing to the issue I opened about --emit and --out-dir?

Sure, I'd at least cc the issue -- if I came across this code I'd probably be curious about the --out-dir=$(TMPDIR).

@bors delegate=abonander

@bors
Copy link
Contributor

bors commented Mar 18, 2017

✌️ @abonander can now approve this pull request

@abonander
Copy link
Contributor Author

@bors: r=jseyfried

Tested it locally, forgot to push before I went out for dinner.

@bors
Copy link
Contributor

bors commented Mar 19, 2017

📌 Commit 74d6cce has been approved by jseyfried

@brson brson removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Mar 20, 2017
bors added a commit that referenced this pull request Mar 20, 2017
[beta] Backports and version bump

This PR backports these PRs to beta:

* #40583
* #40398
* #40542

and it also includes a version bump to push out a beta with all recent backports.
@bors
Copy link
Contributor

bors commented Mar 21, 2017

🔒 Merge conflict

@bstrie
Copy link
Contributor

bstrie commented Mar 21, 2017

@bors r=jseyfried

@bors
Copy link
Contributor

bors commented Mar 21, 2017

📌 Commit 8a6ef50 has been approved by jseyfried

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 23, 2017
Correctly get source for metatdata-only crate type

Closes rust-lang#40535

However, I'm not sure how to approach writing a regression test since I'm still working on a reduced test case from the code that caused the ICE in the first place. It's not enough to have an unknown `extern crate` in a metadata crate, it depends on a few extra arguments but I'm not sure which yet.

Also replaced the `unwrap()` with a more informative `expect()`.

r? @jseyfried
bors added a commit that referenced this pull request Mar 23, 2017
Rollup of 6 pull requests

- Successful merges: #39891, #40518, #40542, #40617, #40678, #40696
- Failed merges:
@bors bors merged commit 8a6ef50 into rust-lang:master Mar 23, 2017
@infinity0
Copy link
Contributor

This test is failing on 1.21.0 on Debian on amd64 and ppc64el:

/<<BUILDDIR>>/rustc-1.21.0+dfsg1/build/x86_64-unknown-linux-gnu/stage2/bin/rustc: error while loading shared libraries: librustc_driver-ec7cf42bd86068a6.so: cannot open shared object file: No such file or directory
make[2]: *** [all] Error 127

Any clue what's causing it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants