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

Take the original extra-filename passed to a crate into account when resolving it as a dependency #49253

Merged
merged 2 commits into from
Apr 5, 2018

Conversation

chmanchester
Copy link
Contributor

resolving it as a dependency.

Fixes #46816

@chmanchester
Copy link
Contributor Author

Hi @alexcrichton, it looks like I haven't triggered the bot that usually assigns reviewers on PRs, have I missed a step here? Thanks!

@alexcrichton alexcrichton self-assigned this Mar 23, 2018
@alexcrichton
Copy link
Member

Thanks for the PR!

I think though we may want a slightly relaxed logic here as well in terms of this PR as-is is technically a breaking change unfortunately. Today we allow renaming the library (aka changing the final hash) and the compiler will continue to look it up and find it. Could we perhaps default to preferring exact matches, but if none are found fall back to the original logic of no hash present?

Additionally, could tests be added for this pr?

@shepmaster shepmaster changed the title Take the original extra-filename passed to a crate into account when Take the original extra-filename passed to a crate into account when resolving it as a dependency Mar 23, 2018
@shepmaster shepmaster added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Mar 23, 2018
@chmanchester
Copy link
Contributor Author

Thank you for the feedback @alexcrichton. I've made the requested change and added a test for the behavior described that we should preserve so this isn't a breaking change. Do you have any tips for how we might implement a more general test for the issue? I'm sort of at a loss for how to detect which crates' metadata is read during a test, but I'm not very familiar with the various test types in rustc.

@alexcrichton
Copy link
Member

Sure yeah! I think we'd want to add a src/test/run-make-fulldeps test which allows us to run custom rustc commands. I think you could generate an rlib using extra-filename and then copy that rlib to a different extra-filename, and that should generate an error today (duplicate libs) but work with this patch? Alternately you could compile two crates with the same crate name to two different crate hashes, and today I think it'll cause an error but work after this PR

@bors
Copy link
Contributor

bors commented Mar 28, 2018

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

@chmanchester
Copy link
Contributor Author

Unfortunately the locator is only emitting the duplicate libs error in the case it finds otherwise matching libraries with distinct hashes, which only happens if the svh isn't included in the search (if it is, either the hash matches and it just picks one, or the hash doesn't match and the candidate is rejected). Transitive dependencies always include the svh in their search, and are the only ones we have the extra-filename for, so I'm having trouble figuring out how to make this fail...

@chmanchester chmanchester force-pushed the probing_fix branch 2 times, most recently from 342f9f1 to 2e6b3b2 Compare March 29, 2018 03:50

all:
$(RUSTC) -C extra-filename=-hash foo.rs
$(RUSTC) bar.rs
Copy link
Member

Choose a reason for hiding this comment

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

If you pass --crate-name foo here, does that make the test fail on today's compiler?

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 does, but because it's identical to one of its dependencies (same crate name and metadata), which is also a failure with this patch.

Copy link
Member

Choose a reason for hiding this comment

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

Could a different metadata be passed to bar.rs here so that error path wouldn't get hit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but then we fail because baz.rs can't tell which version of foo to use, which is still an issue with this patch because we don't yet have an extra-filename for foo, because we're searching for it as a direct dependency of baz. This seemed plausible:

$(RUSTC) -C extra-filename=-hash foo.rs
$(RUSTC) --crate-name foo -C metadata=baz -C extra-filename=-another-hash bar.rs
$(RUSTC) --extern foo=$(TMPDIR)/libfoo-another-hash.rlib baz.rs
$(RUSTC) qux.rs

where qux depends on baz, but even then locator.rs doesn't produce an error because it can differentiate between foo crates by the SVH it finds in the metadata for deps of baz, and doesn't consider the foo found in foo.rs when resolving dependencies for baz.rs.

After trying this and a few more things I'm sort of convinced we're not going to get further in this direction, because whenever extra_filename is present in the locate context, hash will be too, and in this case locator.rs doesn't produce the kind of errors we're trying to induce.

Should we consider landing this without the test?

Copy link
Member

Choose a reason for hiding this comment

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

Ah I think you're right, that makes sense to me and means that it may be quite hard to add a test. In that case I think it's fine, do you want to remove this one as it's not testing what we thought it was testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test may still be interesting, because it's a case that fails with an earlier version of this patch (which didn't fall back to an imprecise match if the match considering extra-filename failed), but I'd be ok with leaving it out for now as well. Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Oh ok! If it catches a previous mistake sounds good to leave in

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 3, 2018

📌 Commit bd81547 has been approved by alexcrichton

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 3, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this pull request Apr 5, 2018
…chton

Take the original extra-filename passed to a crate into account when resolving it as a dependency

resolving it as a dependency.

Fixes rust-lang#46816
kennytm added a commit to kennytm/rust that referenced this pull request Apr 5, 2018
…chton

Take the original extra-filename passed to a crate into account when resolving it as a dependency

resolving it as a dependency.

Fixes rust-lang#46816
bors added a commit that referenced this pull request Apr 5, 2018
Rollup of 9 pull requests

Successful merges:

 - #48658 (Add a generic CAS loop to std::sync::Atomic*)
 - #49253 (Take the original extra-filename passed to a crate into account when resolving it as a dependency)
 - #49345 (RFC 2008: Finishing Touches)
 - #49432 (Flush executables to disk after linkage)
 - #49496 (Add more vec![... ; n] optimizations)
 - #49563 (add a dist builder to build rust-std components for the THUMB targets)
 - #49654 (Host compiler documentation: Include private items)
 - #49667 (Add more features to rust_2018_preview)
 - #49674 (ci: Remove x86_64-gnu-incremental builder)

Failed merges:
@bors bors merged commit bd81547 into rust-lang:master Apr 5, 2018
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.

4 participants