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

Support linking of rlib files in rustpkg #11500

Merged
merged 2 commits into from
Jan 22, 2014
Merged

Conversation

jhasse
Copy link
Contributor

@jhasse jhasse commented Jan 12, 2014

Currently rustpkg only looks for shared libraries. After this patch it also looks for *.rlib files.

@alexcrichton
Copy link
Member

Hm, I'm a little uncomfortable committing this because this seems to me like a clear indication that rustpkg is duplicating functionality that it shouldn't. This has different behavior when finding duplicate rlibs and duplicate dylibs which seems surprising to me.

Can you look into whether rustc::metadata::loader can be used for this?

@jhasse
Copy link
Contributor Author

jhasse commented Jan 12, 2014

I thought exactly the same, but thought there must be a reason for this to be rewritten in rustpkg. I will look into it ;)

btw: There seems to be more places with duplicated functionality in rustpkg: #11502

@jhasse
Copy link
Contributor Author

jhasse commented Jan 12, 2014

Okay what I found so far: The function metadata::loader::Context::find_library_crate does about the same as library_in. But it also does more, e.g. extracting the metadata. So using it in rustpkg doesn't look like a good idea to me, since then the same work will be done two times, the first time by rustpkg and the second by librustc. And extracting the metadata by rustpkg is unnecessary.

A solution could be to use library_in in find_library_crate (and since rustc shouldn't depend on librustpkg move library_in to librustc). Still the file system search would then be performed two times.

The best solution would be to further integrate rustc's build process and rustpkg. E.g. when rustpkg finds the libraries it could already pass the Library struct to librustc's functions. Or rustpkg doesn't even look for libraries and let's rustc handle this.

Unfortunately I don't see myself skilled enough in Rust to implement either solution (also lacking enough time to do it). Maybe merge this and see where rustpkg is going in the future anyway?

@alexcrichton
Copy link
Member

I guess I'm a little unclear on why rustpkg is trying to find libraries, do you know why? I would imagine that to be correct rustpkg must open and read the metadata to ensure that it has the right version, but it's not doing that?

@metajack
Copy link
Contributor

It tries to search for outputs because it didn't use the logic to calculate the crate hash. That part can be easily fixed now.

I'm not sure why it also searches inputs, but I suspect because rustc does not know about RUST_PATH and there might be many places to search. Perhaps @catamorphism remembers.

@klutzy
Copy link
Contributor

klutzy commented Jan 13, 2014

rustpkg doesn't compute hash because it was impossible before #10593, and rustpkg hasn't updated. (#11478 has a patch for the issue.)

bors added a commit that referenced this pull request Jan 22, 2014
Currently `rustpkg` only looks for shared libraries. After this patch it also looks for `*.rlib` files.
@bors bors closed this Jan 22, 2014
@bors bors merged commit 6ccc1e8 into rust-lang:master Jan 22, 2014
@jhasse jhasse deleted the patch-rlib branch January 22, 2014 16:54
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.

5 participants