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

[revert 6193] Remove Compilation.libraries #6517

Closed
wants to merge 1 commit into from

Conversation

gnzlbg
Copy link
Contributor

@gnzlbg gnzlbg commented Jan 3, 2019

This PR reverts #6193 .

It appears that the only reason for the removal was that this API was unused, but as pointed out in the original PR, Rust projects are using this API.

No deprecation warning indicating the migration path was issued (there was a non-doc comment indicating that the API was deprecated, but that's it, no deprecation() attribute was used at all).

#6193 cites #5651 for the removal, but this comment there also states that there are projects using this API and therefore "I'm not sure if we should just leave it, remove it, or replace it with something better". No replacement is discussed anywhere in those PRs and AFAICT no such replacement is available.

Please, do a cargo release after this PR is merged.

cc @dwijnand

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nrc (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@dwijnand
Copy link
Member

dwijnand commented Jan 3, 2019

I'm not sure if this is stated anywhere formal, but Cargo doesn't have a public API so depending on its programmatic API is at your own risk and without deprecation warnings. The CLI is what's stable.

I think the output of cargo metadata is intended for programmatic consumption, so you might want to look at that.

I don't think we should revert the removal.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jan 3, 2019

I don't think we should revert the removal.

I see! I'll explore whether it is possible to migrate rust-semverver to cargo metadata in the future, in my little experience with cargo metadata I doubt that it will be possible to do so easily.

For the people of the future, I've published a fork of cargo (https://github.com/gnzlbg/cargo/tree/cargo_semverver) with this patch under cargo-semverver on crates.io in case other projects need this functionality ! Sorry for the noise!

@gnzlbg gnzlbg closed this Jan 3, 2019
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jan 4, 2019

I think the output of cargo metadata is intended for programmatic consumption, so you might want to look at that.

FWIW I explored using cargo metadata more in-depth today, but AFAICT the output of cargo metadata does not contain any information about the output paths of rlibs, for example (or any output paths of any build artifacts whatsoever).

So it appears that we are going to have to maintain our own fork of cargo for the forseable future.

@ehuss
Copy link
Contributor

ehuss commented Jan 4, 2019

output paths of rlibs

The JSON output from cargo build is probably the best way to obtain this.

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