Skip to content

Downloading a single package is much more painful on master than on 0.31 #6518

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

Closed
gnzlbg opened this issue Jan 3, 2019 · 8 comments · Fixed by #6637
Closed

Downloading a single package is much more painful on master than on 0.31 #6518

gnzlbg opened this issue Jan 3, 2019 · 8 comments · Fixed by #6637
Assignees
Labels
C-bug Category: bug

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Jan 3, 2019

Problem

Downloading a single package in cargo 0.31 was as simple as:

let package_id: PackageId;
let package = source.source.download(&package_id)?;

With cargo master one needs to:

let package_ids: [PackageId];  
let source: Box<Source + 'a>;
let config: Config;
let sources = {
    let mut s = SourceMap::new();
    s.insert(source);
    s
};

let package_set = PackageSet::new(&package_ids, sources, config)?;
let mut downloads = package_set.enable_download()?;
let package = if let Some(package) = downloads.start(package_ids[0])? {
    package
} else {
    downloads.wait()?;
    downloads.start(package_ids[0])?.expect("started download did not finish after wait")
};

This feels like a big API regression.

@gnzlbg gnzlbg added the C-bug Category: bug label Jan 3, 2019
@alexcrichton
Copy link
Member

FWIW get_one was added as a convenience to do all this

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jan 3, 2019

Thanks! get_one cuts it down to:

let package_ids: [PackageId];  
let source: Box<Source + 'a>;
let config: Config;
let sources = {
    let mut s = SourceMap::new();
    s.insert(source);
    s
};

let package_set = PackageSet::new(&package_ids, sources, config)?;
let package = package_set.get_one(package_ids[0])?;

@dwijnand
Copy link
Member

dwijnand commented Jan 3, 2019

I feel bad for the libraries removal thing. I'd be happy to try and land some improvements to the API if you have any recommendations...

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jan 3, 2019

@dwijnand I pretty much agree with you that we should be able to do that via the cargo metadata somehow. I've opened an issue about that in the cargo-metadata bindings (oli-obk/cargo_metadata#60 ) and it might make more sense to improve the bindings to support that, also improving cargo metadata as required.

We can revisit adding a better API to cargo for that if the cargo metadata approach ends up being too complicated.

@dwijnand
Copy link
Member

dwijnand commented Jan 3, 2019

Good to hear!

But I actually was thinking for the single download API. Perhaps source.source could have a download method that does the necessary packageset/sourcemap setup to download a package.

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jan 3, 2019

That would help!

@dwijnand
Copy link
Member

dwijnand commented Feb 5, 2019

So I was looking back at this, trying to understand what cargo API rust-semverver used to use that's now gone. And I'm confused because source.download(&package_id) still exists:

/// The download method fetches the full package for each name and
/// version specified.
fn download(&mut self, package: PackageId) -> CargoResult<MaybePackage>;

So going back to the outset, why doesn't that work any more?

@ehuss
Copy link
Contributor

ehuss commented Feb 5, 2019

@dwijnand It was changed to return a MaybePackage, so it doesn't actually download. PackageSet can be used to download now.

@dwijnand dwijnand self-assigned this Feb 5, 2019
bors added a commit that referenced this issue Feb 9, 2019
@bors bors closed this as completed in #6637 Feb 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants