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

Do not rely on file extensions after path canonicalization. #32828

Merged
merged 2 commits into from Apr 13, 2016

Conversation

vadimcn
Copy link
Contributor

@vadimcn vadimcn commented Apr 8, 2016

Rustc does not recognize libraries which are symlinked to files having extension other than .rlib. The problem is that find_library_crate calls fs::canonicalize on found library paths, but then the resulting path is passed to get_metadata_section, which assumes it will end in ".rlib" if it's an rlib (from https://internals.rust-lang.org/t/is-library-path-canonicalization-worth-it/3206).

cc #29433

@rust-highfive
Copy link
Collaborator

r? @nrc

(rust_highfive has picked a reviewer for you, use r? to override)

@vadimcn
Copy link
Contributor Author

vadimcn commented Apr 8, 2016

Also, I'd like to nominate this fix for backporting into 1.8.

fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
f.write_str(match *self {
CrateFlavor::Rlib => "rlib",
CrateFlavor::Dylib => "dylib"
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 add a comma at the end of this line please?

@alexcrichton
Copy link
Member

This seems pretty similar to #32317 in scope, but perhaps somewhat different in terms of problem solved? Could you also be sure to add a test for this as well?

@alexcrichton alexcrichton assigned alexcrichton and unassigned nrc Apr 8, 2016
@vadimcn
Copy link
Contributor Author

vadimcn commented Apr 8, 2016

No, I think it's the same issue. Wasn't aware #32317 existed.

@alexcrichton
Copy link
Member

I think that #32317 is arguably more general, but I somewhat prefer this tactic as it solves the problem at hand without changing the semantics that much

@alexcrichton
Copy link
Member

cc @taralx

@vadimcn
Copy link
Contributor Author

vadimcn commented Apr 9, 2016

Added a test in case you want to go with this one.

@taralx
Copy link
Contributor

taralx commented Apr 9, 2016

This fixes the problem that I have specifically, although #32317 is probably also worth doing long-term (once I work out what's broken about Windows with it).

@alexcrichton
Copy link
Member

@bors: r+ cc3b6f2

Ok, thanks @vadimcn! @taralx yeah I think we'll want to take both strategies, so I'll merge this for now while you're working on #32317, and we can merge once the Windows issue is tracked down.

@bors
Copy link
Contributor

bors commented Apr 13, 2016

⌛ Testing commit cc3b6f2 with merge 525aa61...

bors added a commit that referenced this pull request Apr 13, 2016
Do not rely on file extensions after path canonicalization.

Rustc does not recognize libraries which are symlinked to files having extension other than .rlib. The problem is that find_library_crate calls fs::canonicalize on found library paths, but then the resulting path is passed to get_metadata_section, which assumes it will end in ".rlib" if it's an rlib (from https://internals.rust-lang.org/t/is-library-path-canonicalization-worth-it/3206).

cc #29433
@bors bors merged commit cc3b6f2 into rust-lang:master Apr 13, 2016
@eddyb eddyb added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 13, 2016
@eddyb
Copy link
Member

eddyb commented Apr 13, 2016

Nominating for backport on behalf of @taralx.

@nikomatsakis
Copy link
Contributor

Discussed in @rust-lang/compiler meeting. Is this fixing a regression or just some longstanding bug?

@taralx
Copy link
Contributor

taralx commented Apr 21, 2016

This is technically a regression due to #12474. Before that, symlinks named *.rlib worked, regardless of the name of the thing they pointed to.

@alexcrichton
Copy link
Member

@nikomatsakis I believe, (ias @taralx pointed out) that this is just fixing a longstanding bug

@nikomatsakis
Copy link
Contributor

@alexcrichton

I believe that this is just fixing a longstanding bug

In that case, I believe (based on consensus from previous @rust-lang/compiler meeting) we would prefer not to backport.

@nikomatsakis nikomatsakis removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Apr 27, 2016
@vadimcn
Copy link
Contributor Author

vadimcn commented Apr 27, 2016

@rust-lang/compiler: Can you please reconsider? This prevents Rust builds from working on our build cluster. I believe this is a pretty non-invasive change, and the beta cycle has just started....

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants