Correctly get source for metatdata-only crate type by abonander · Pull Request #40542 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation39 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

abonander

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 titleCorrectly get source for metadata crate type Correctly get source for metatdata-only crate type

Mar 15, 2017

@arielb1

@nikomatsakis

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

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

@abonander

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

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

@abonander

@arielb1

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

@abonander

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

@abonander

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

@abonander

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

jseyfried

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

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)

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

@jseyfried

@bors

📌 Commit e670335 has been approved by jseyfried

@abonander

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

Not sure how to fix this.

@jseyfried

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

@abonander

Why is it not writing there now then? Weird.

@jseyfried

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

@arielb1

@abonander

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

jseyfried

$(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=(RUSTC) baz.rs --emit=metadata --out-dir=(RUSTC)baz.rs−−emit=metadata−−out−dir=(TMPDIR)

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...

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.

Choose a reason for hiding this comment

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

@abonander

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

@alexcrichton

@bors: r=jseyfried

What weird bug!

@bors

📌 Commit 3cbdb8f has been approved by jseyfried

@abonander

@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

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

jseyfried

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

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.

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

@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

@abonander

@bors: r=jseyfried

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

@bors

📌 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

@bors

[beta] Backports and version bump

This PR backports these PRs to beta:

and it also includes a version bump to push out a beta with all recent backports.

@bors

@abonander @retep998

replace unwrap() with expect()

@abonander @retep998

@bstrie

@bors

📌 Commit 8a6ef50 has been approved by jseyfried

frewsxcv added a commit to frewsxcv/rust that referenced this pull request

Mar 23, 2017

@frewsxcv

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

@bors

This was referenced

Mar 25, 2017

@infinity0

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?

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.