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

Re-export pep404 #19

Closed
gaborbernat opened this issue Oct 16, 2024 · 11 comments · Fixed by #20
Closed

Re-export pep404 #19

gaborbernat opened this issue Oct 16, 2024 · 11 comments · Fixed by #20

Comments

@gaborbernat
Copy link

In https://github.com/tox-dev/pyproject-fmt-rust I am running into problems because of this line https://github.com/tox-dev/pyproject-fmt-rust/blob/d94116328fda35f8dd1a9caf8af755b9a6427ab4/rust/src/helpers/pep508.rs#L26. The issue boils down to the fact that the version of the pep440 I define is different from what this lib uses, and this behaviour is expected as detailed at https://doc.rust-lang.org/cargo/reference/resolver.html#version-incompatibility-hazards.

According to https://stackoverflow.com/questions/72366391/how-to-let-a-package-use-the-same-version-of-dependencies-as-b the best practice here in rust is to re-export these objects that are part of the public API. So I am asking here to do so.

@konstin
Copy link
Owner

konstin commented Oct 16, 2024

For this case, you probably manually have to check that pep440_rs and pep508_rs have compatible versions (pep508_rs is lagging a bit behind atm), at least we had to do this other packages, but I'll check with an expert.

@gaborbernat
Copy link
Author

gaborbernat commented Oct 16, 2024

Yeah, that what I had to do, and it is a bit error-prone because when upgrading can get unexpected errors unless the versions align up. As I do not use that lib directly, it is better to expose it IMHO, is a simple:

pub use serde pep440_rs;

inside https://github.com/konstin/pep508_rs/blob/main/src/lib.rs#L34 and then people can refer to it via pep508_rs::pep440_rs:: removing a whole source of errors :D and is explicit that I want pep440_rs as used inside pep508_rs.

@gaborbernat
Copy link
Author

@konstin ping on this?

@BurntSushi
Copy link

@gaborbernat Can you say more about the specific issue you're hitting here? I know you linked to a line of code, but it's not totally clear to me, from that link, what the specific issue you're hitting is.

@gaborbernat
Copy link
Author

It is very simple. Unless you manually align up the two versions You are getting completion error because Rust thinks the two objects are not the same when making compression between an object a method returns from this library and a constant I am using to make the comparison.

@BurntSushi
Copy link

BurntSushi commented Oct 28, 2024

Okay, so let's be concrete about the workflow here that leads to a problem:

  • Your project depends on pep440_rs 0.x and pep508_rs 0.y.
  • Both pep440_rs 0.(x+1) and pep508_rs 0.(y+1) are released, with the latter depending on the former.
  • You go to upgrade pep508_rs, which causes both pep440_rs 0.x and pep440_rs 0.(x+1) to be in your dependency tree at the same time.
  • This in turn leads to code trying to use types from both pep440_rs 0.x and pep440_rs 0.(x+1) simultaneously as if they were identical, but of course, they are not.

Is that right? Or is the workflow different from that? I suppose the above implies that you have to do the upgrades to pep440_rs and pep508_rs simultaneously. You can't upgrade them independently.


I am overall in favor of re-exporting public dependencies in general. I think it's a good idea. There really isn't much downside AFAIK, and generally helps issues like this. Or at least, gives you a way out. But if the above workflow is an accurate reflection of your problem, then I do believe it would require that all uses of pep440_rs on your end are done through pep508_rs. If you're still explicitly depending on pep440_rs somewhere in your dependency tree, then that upgrade may still need to be done in concert with any pep508_rs upgrades.

@gaborbernat
Copy link
Author

gaborbernat commented Oct 28, 2024

It is much simpler than that. Your project depends on this library. You call a public method from this library and want to check if the return value of that is a given value. The method in question returns the value from pep440. In the current situation, the only way Rust will allow you to do this is by adding an explicit dependency on pep440. However, should you at some point in time Set the version of pep440 library to something else than what this project set during its release. You will get a compilation error because oddly enough, Rust thinks the two objects are not the same. The error message is very confusing and takes some time to understand what's happening. This is actually very easily achievable if you use cargo update to update your dependencies because there might often be a newer version of the PEP 440 library than what this library uses.

Did you read the stack overflow issue I linked? Explained the problem, I think pretty well.

@BurntSushi
Copy link

Yes. I understand the general shape of the problem. I'm more or less trying to understand how it specifically manifests for you. Thanks for explaining! I think what you're saying is the same as what I said? At least, I can't spot any differences except for one: doing a cargo update just by itself should not result in a mis-compilation due to this issue. That's why I'm asking how this manifests specifically, because if just doing a cargo update caused a problem for you, then it's likely that something else is going wrong somewhere. For example, perhaps there was a semver incompatible bump in a public dependency in a semver compatible release. i.e., A bug in the pep508_rs release process. cc @konstin if that happened? I dunno. A quick look at the commit history suggests not.

@gaborbernat
Copy link
Author

For me, in particular, I think the problem was that this library was using version 0.6.8, but there was a 0.7.0 already released, so when bumped to it, the problem manifested. Rust doesn't do semantic version compatibility. If there are two libraries, even with a minor difference, they are treated as two separate objects.

@BurntSushi
Copy link

If you have pep440_rs = "0.6.8" in your Cargo.toml, then Cargo will treat 0.7.0 as semver incompatible with that. And therefore, a cargo update on its own will not automatically upgrade your pep440_rs dependency to 0.7.0. (See the Cargo docs on SemVer compatibility for more info.)

Now, if you update pep440_rs = "0.6.8" to pep440_rs = "0.7.0" manually without updating pep508_rs, then yes, that will indeed lead to problems. That's why I mentioned needing to update both simultaneously. And if pep508_rs re-exports pep440_rs, then the only way it solves your problem is if you remove pep440_rs from your Cargo.toml and specifically use pep440_rs through pep508_rs's re-export.

@gaborbernat
Copy link
Author

And if pep508_rs re-exports pep440_rs, then the only way it solves your problem is if you remove pep440_rs from your Cargo.toml and specifically use pep440_rs through pep508_rs's re-export.

That's what I want. The only reason I added pep440 as a dependency is to check the return value of a public method of this library is of a given type. So If I can finally not do that, that would be grand.

konstin added a commit that referenced this issue Oct 29, 2024
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 a pull request may close this issue.

3 participants