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

Upgrade dependencies #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

upsuper
Copy link

@upsuper upsuper commented May 12, 2017

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
Copy link
Author

upsuper commented May 12, 2017

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

@upsuper
Copy link
Author

upsuper commented May 12, 2017

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

@est31
Copy link
Contributor

est31 commented May 28, 2017

@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: servo/servo#17067

@upsuper
Copy link
Author

upsuper commented May 28, 2017

@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
Copy link
Author

upsuper commented May 28, 2017

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

@est31
Copy link
Contributor

est31 commented May 28, 2017

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 servo/servo#16556

@est31 est31 mentioned this pull request May 28, 2017
@joshtriplett
Copy link
Owner

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
Copy link
Owner

joshtriplett commented Jun 2, 2017

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.

@upsuper
Copy link
Author

upsuper commented Oct 3, 2017

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?

@joshtriplett
Copy link
Owner

joshtriplett commented Oct 3, 2017

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

Also see rust-lang/libz-sys#12

@upsuper
Copy link
Author

upsuper commented Oct 4, 2017

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.

@joshtriplett
Copy link
Owner

@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.

@nox
Copy link

nox commented Nov 16, 2017

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

@upsuper
Copy link
Author

upsuper commented Nov 16, 2017

@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.

@joshtriplett
Copy link
Owner

joshtriplett commented Nov 16, 2017

@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 rust-lang/rfcs#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.

@upsuper
Copy link
Author

upsuper commented Nov 16, 2017

@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.

@nox
Copy link

nox commented Nov 17, 2017

@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.

@gdesmott gdesmott mentioned this pull request Apr 30, 2020
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.

4 participants