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

Remove Compilation.libraries #6193

Merged
merged 1 commit into from
Oct 20, 2018

Conversation

dwijnand
Copy link
Member

Actioning #5651 (comment)

@rust-highfive
Copy link

r? @alexcrichton

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

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Collaborator

bors commented Oct 20, 2018

📌 Commit 6c6947b has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Oct 20, 2018

⌛ Testing commit 6c6947b with merge c8c4ff8...

bors added a commit that referenced this pull request Oct 20, 2018
@bors
Copy link
Collaborator

bors commented Oct 20, 2018

💔 Test failed - status-appveyor

@ehuss
Copy link
Contributor

ehuss commented Oct 20, 2018

For context, I left that in when I removed its use in #5651 because it was used by cargo-bloat and semverver. It looks like cargo-bloat has since been rewritten to no longer use cargo.

@dwijnand
Copy link
Member Author

The failure looks spurious btw.

@alexcrichton
Copy link
Member

@bors: retry

@bors
Copy link
Collaborator

bors commented Oct 20, 2018

⌛ Testing commit 6c6947b with merge 2d0863f...

bors added a commit that referenced this pull request Oct 20, 2018
@bors
Copy link
Collaborator

bors commented Oct 20, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 2d0863f to master...

@bors bors merged commit 6c6947b into rust-lang:master Oct 20, 2018
@dwijnand dwijnand deleted the remove-Compilation.libraries branch October 21, 2018 10:23
@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 3, 2019

For context, I left that in when I removed its use in #5651 because it was used by [...] semverver.

So what should rust-semverver use now to find the path of rlibs from a PackageId now that libraries has been removed ?

Since the usage of this field by rust-semverver was known, I suppose there must be a way to do that, but after staring at this and related PRs for 45 minutes and the cargo docs, I have no idea how to do that. It would have been nice to give that project a heads up, e.g. by filling an issue, with migration instructions, so that one doesn't have to spend this much time trying to figure out things 2 months later..

@ehuss
Copy link
Contributor

ehuss commented Jan 3, 2019

So what should rust-semverver use now to find the path of rlibs from a PackageId now that libraries has been removed ?

The issue is that there is not a simple, correct mapping anymore. There might be multiple lib targets with different settings.

I took a look at the semverver code for a bit. I'm wondering if it really needs to use cargo as a library. Would it be possible to just call the cargo cli, and get the rlib path from the JSON output?

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 3, 2019

I'm wondering if it really needs to use cargo as a library. Would it be possible to just call the cargo cli, and get the rlib path from the JSON output?

rust-semverver compiles the current crate as an rlib, as well as the previous version of it from crates.io (which has often to be downloaded, and then also compiled), and then compares both rlibs. The compilation is done by calling cargo::ops::compile.

I've opened an issue about this on cargo_metadata: oli-obk/cargo_metadata#60 , there is some discussion there about how to tackle some of these issues.

@ehuss ehuss added this to the 1.31.0 milestone Feb 6, 2022
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

6 participants