metadeps icon indicating copy to clipboard operation
metadeps copied to clipboard

Upgrade dependencies

Open upsuper opened this issue 8 years ago • 18 comments

This updates:

  • error-chain from 0.7.1 to 0.10.x, which doesn't require any code change
  • toml from 0.2.x to 0.4.x, which removes toml::Value::lookup and thus need to expand the code a bit

upsuper avatar May 12 '17 06:05 upsuper

Not sure whether it needs a patch, minor, or major version up.

upsuper avatar May 12 '17 06:05 upsuper

It seems toml 0.4.x doesn't include any features, so I remove the default-features = false as well.

upsuper avatar May 12 '17 07:05 upsuper

@joshtriplett could you have a look? It would also be nice if you could bump the version of the crate (prefferably just a minor version bump).

This PR and a version bump are needed for a servo PR: https://github.com/servo/servo/pull/17067

est31 avatar May 28 '17 02:05 est31

@est31 For Servo, I pretty much believe you want to open another PR which only upgrades error-chain, because the new version of toml requires serde 1.0, and for having that, you would have to upgrade a good number of Servo dependencies.

upsuper avatar May 28 '17 02:05 upsuper

If you can upgrade all of Servo dependencies to serde 1.0... that would be great as well :)

upsuper avatar May 28 '17 02:05 upsuper

the new version of toml requires serde 1.0, and for having that, you would have to upgrade a good number of Servo dependencies.

Thanks for pointing out! That would be https://github.com/servo/servo/issues/16556

est31 avatar May 28 '17 02:05 est31

I'm going to hold off on merging the toml upgrade for a little bit, as it sounds like that'll make things easier for the Servo folks. I'll merge the error-chain upgrade via #2.

joshtriplett avatar May 30 '17 06:05 joshtriplett

I'm actually a little hesitant to upgrade to toml 0.4, because the new version now unconditionally depends on serde. One of the requirements of metadeps is to add minimal dependencies beyond pkg-config itself; I've actually thought about dropping error-chain for that reason. While a crate that already depends on serde wouldn't have a problem with it, I don't want lower-level libraries to hesitate to use metadeps because it pulls in serde when they didn't otherwise need it. I've had crate owners push back on using metadeps just because of the dependencies it already has, even without adding more.

I'm going to make a post on the forum about this, and seek feedback. I'd also welcome feedback on this issue.

joshtriplett avatar Jun 02 '17 18:06 joshtriplett

https://internals.rust-lang.org/t/metadeps-and-dependencies/5349 https://www.reddit.com/r/rust/comments/6ewn7v/metadeps_and_dependencies/

joshtriplett avatar Jun 02 '17 19:06 joshtriplett

So Servo has finished upgrading its dependencies to Serde, and I think this becomes a blocker for us to upgrade toml crate.

I do understand and appreciate the effort to avoid introducing more dependencies to low-level crates. toml doesn't introduce anything other than Serde, and given that no concern was raised in the threads you posted, I guess it is probably fine to proceed with this PR?

upsuper avatar Oct 03 '17 05:10 upsuper

@upsuper Is it not possible for Servo to keep including the previous version of toml as well, for the time being?

Also see https://github.com/alexcrichton/libz-sys/pull/12

joshtriplett avatar Oct 03 '17 13:10 joshtriplett

We have whitelist to allow duplicate crates, but those are general for small and widely-used crates, so I don't think toml would fit in that category.

Actually it makes me think that we probably shouldn't put toml crate itself as a dependency in such lower-level libraries at all.

Probably we should just put the dependencies in a macro so that they can be written in a declarative way in build.rs rather than Cargo.toml. I don't see how putting it in Cargo.toml is better than in build.rs given that you need to have the latter anyway.

If we go the macro route, we probably don't need any dependency, just like pkg-config.

upsuper avatar Oct 04 '17 05:10 upsuper

@upsuper The goal, as metadeps migrates into something better supported by Cargo, will be to eliminate build.rs entirely in the majority of crates. So putting the dependencies in a macro in build.rs would be problematic.

I do think that eventually we should eliminate the toml dependency; ideally, Cargo would hand the (meta)build script a parsed structure from the toml.

joshtriplett avatar Oct 08 '17 08:10 joshtriplett

@joshtriplett What is blocking this? Can we get the toml bump now?

nox avatar Nov 16 '17 08:11 nox

@nox I think the issue is that @joshtriplett doesn't want to introduce more dependencies for this low-level crate, and that probably makes sense. I guess it is probably better to have another crate doing everything inside build.rs directly, and move our dependencies to that crate instead of using metadeps, so that we can remove the toml dependency from this crate.

upsuper avatar Nov 16 '17 19:11 upsuper

@upsuper Personally, I don't have a problem with a dependency on serde, but I have had people push back on using metadeps because of its dependencies.

Long-term, my plan is a combination of https://github.com/rust-lang/rfcs/pull/2196 and changing Cargo to pass this data to metabuild scripts in some structured, pre-parsed form. That will eliminate the need for any parser.

Short-term, if you have a critical need to eliminate the old toml from your build immediately, you should probably use pkg-config directly. If that's not an option for you, such as because one of your dependencies uses metadeps, then please go ahead and include toml 0.2 in your build. toml is a "small, widely used crate". If you feel that this is impacting you to the point that you're considering pushing one of your dependencies to drop metadeps, please let me know and I'll try to accelerate a solution.

joshtriplett avatar Nov 16 '17 19:11 joshtriplett

@joshtriplett There is currently no critical reason for me to use the new version of toml (which is currently used by style crate's build script), but I'm not sure whether @nox wants to do something else with the new version of toml.

upsuper avatar Nov 16 '17 19:11 upsuper

@joshtriplett There is currently no critical reason for me to use the new version of toml (which is currently used by style crate's build script), but I'm not sure whether @nox wants to do something else with the new version of toml.

It's the same thing as usual, trying to reduce duplicate dependencies in Servo's large dependency tree. We already build toml 0.4 so I would rather not also build toml 0.2.

nox avatar Nov 17 '17 09:11 nox