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

RFC for Public/Private Dependencies #1977

Merged
merged 13 commits into from Sep 17, 2017

Conversation

@mitsuhiko
Contributor

mitsuhiko commented Apr 18, 2017

This RFC introduces the concept of a public/private separation of crate dependencies to aid the ecosystem to explicitly deal with dependencies that themselves become part of the public API of a crate.

Rendered

Show outdated Hide outdated text/0000-public-private-dependencies.md
dependencies as public.
**Q: Can I export a type from a private dependency as my own?**<br>
For now it will not be strictly permissible to privately depend on a crate and export

This comment has been minimized.

@untitaker

untitaker Apr 18, 2017

Contributor

This appears to contradict the answer to the previous question. Is it or is it not allowed to do this? If not, doesn't this break backwards compat a bit harshly?

@untitaker

untitaker Apr 18, 2017

Contributor

This appears to contradict the answer to the previous question. Is it or is it not allowed to do this? If not, doesn't this break backwards compat a bit harshly?

Show outdated Hide outdated text/0000-public-private-dependencies.md
**Q: Can I export a type from a private dependency as my own?**<br>
For now it will not be strictly permissible to privately depend on a crate and export
a type from their as your own. The reason for this is that at the moment it is not

This comment has been minimized.

@untitaker

untitaker Apr 18, 2017

Contributor

s/their/there/

@untitaker

untitaker Apr 18, 2017

Contributor

s/their/there/

Show outdated Hide outdated text/0000-public-private-dependencies.md
The feature will be called `public_private_dependencies` and it comes with one
lint flag called `external_private_dependency`. For all intents and purposes this
should be the extend of the new terms introduced in the beginning. This RFC however

This comment has been minimized.

@untitaker

untitaker Apr 18, 2017

Contributor

s/extend/extent/

@untitaker

untitaker Apr 18, 2017

Contributor

s/extend/extent/

Show outdated Hide outdated text/0000-public-private-dependencies.md
The feature will be called `public_private_dependencies` and it comes with one
lint flag called `external_private_dependency`. For all intents and purposes this
should be the extend of the new terms introduced in the beginning. This RFC however
lays the groundwork for later providing aliasing so that a private dependencies could

This comment has been minimized.

@untitaker

untitaker Apr 18, 2017

Contributor

s/dependencies/dependency/

@untitaker

untitaker Apr 18, 2017

Contributor

s/dependencies/dependency/

Show outdated Hide outdated text/0000-public-private-dependencies.md
There are a few open questions about how to best hook into the compiler and cargo
infrastructure:
* is passing in the last of public dependencies the correct way to get around it?

This comment has been minimized.

@untitaker

untitaker Apr 18, 2017

Contributor

s/last/list/

@untitaker

untitaker Apr 18, 2017

Contributor

s/last/list/

Show outdated Hide outdated text/0000-public-private-dependencies.md
their code refuses to compile because different versions of those libraries are requested
or where compiler messages are less than clear.
The introduction of an explicit distinction between public and private dependencies can

This comment has been minimized.

@untitaker

untitaker Apr 18, 2017

Contributor

Are there concrete usecases where these annotations would help? Those would be worth mentioning here (primarily to pitch this RFC to unconvinced readers)

At least when I started writing Rust, I upgraded dependencies in a PATCH release without thinking about the implications for reexported APIs. This change could make crate authors think about this from the start. The last bulletpoint in "Unresolved Question" also hints at this.

@untitaker

untitaker Apr 18, 2017

Contributor

Are there concrete usecases where these annotations would help? Those would be worth mentioning here (primarily to pitch this RFC to unconvinced readers)

At least when I started writing Rust, I upgraded dependencies in a PATCH release without thinking about the implications for reexported APIs. This change could make crate authors think about this from the start. The last bulletpoint in "Unresolved Question" also hints at this.

Show outdated Hide outdated text/0000-public-private-dependencies.md
Additionally later on the warning can turn into a hard error in general.
In some situations it can be necessary to allow private dependencies to become

This comment has been minimized.

@untitaker

untitaker Apr 18, 2017

Contributor
  • You already hint at libcore and libstd, but to me this part is missing justification for why we should allow normal crate authors to disable this warning. An example where this becomes necessary might help.
  • At https://github.com/rust-lang/rfcs/pull/1977/files#diff-396833e435181d84da75db4b1f53ef8dR137 you list a few cases where one might think it'd be fine to use this. For example when leaking impls of external traits. Which usecases do we encourage?
@untitaker

untitaker Apr 18, 2017

Contributor
  • You already hint at libcore and libstd, but to me this part is missing justification for why we should allow normal crate authors to disable this warning. An example where this becomes necessary might help.
  • At https://github.com/rust-lang/rfcs/pull/1977/files#diff-396833e435181d84da75db4b1f53ef8dR137 you list a few cases where one might think it'd be fine to use this. For example when leaking impls of external traits. Which usecases do we encourage?

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 19, 2017

Contributor

I rather not go into too much detail about this as this is a big unknown. I think it's largely irrelevant for this RFC anyways because we initially start out with just compiler warnings. Where to go from there will be seen by how much damage this does.

@mitsuhiko

mitsuhiko Apr 19, 2017

Contributor

I rather not go into too much detail about this as this is a big unknown. I think it's largely irrelevant for this RFC anyways because we initially start out with just compiler warnings. Where to go from there will be seen by how much damage this does.

Show outdated Hide outdated text/0000-public-private-dependencies.md
running cargobomb/crater.
* since changing public dependency pins/ranges requires a change in semver it might
be worth exploring if cargo could prevent the user in pushing up new crate
versions that violate that constraint.

This comment has been minimized.

@untitaker

untitaker Apr 18, 2017

Contributor

Not a review, but random thought: You might have a public dependency that changes its major version but doesn't change the part of the API you reexport, or perhaps it just bumps its major version too aggressively.

@untitaker

untitaker Apr 18, 2017

Contributor

Not a review, but random thought: You might have a public dependency that changes its major version but doesn't change the part of the API you reexport, or perhaps it just bumps its major version too aggressively.

This comment has been minimized.

@withoutboats

withoutboats Apr 19, 2017

Contributor

This is still a breaking change in your library.

Suppose you and another crate foo both depend on the same version of bar, and both re-export bar::Bar. I depend on both you and foo, and pass a Bar I got from you to foo in my code. This works fine, because your Bar is the same type as foo's Bar.

bar releases a breaking change, which does nothing to the definition of Bar. You update to that major version, but don't do a breaking change because you believe nothing in your code has changed.

However, now its impossible for cargo to unify your dependency on bar with foo's dependency on bar, because they're different major versions. This means that you::Bar and foo::Bar are no longer the same type. If I upgrade my dependency on you, I will get a breakage, so you have made a breaking change.

@withoutboats

withoutboats Apr 19, 2017

Contributor

This is still a breaking change in your library.

Suppose you and another crate foo both depend on the same version of bar, and both re-export bar::Bar. I depend on both you and foo, and pass a Bar I got from you to foo in my code. This works fine, because your Bar is the same type as foo's Bar.

bar releases a breaking change, which does nothing to the definition of Bar. You update to that major version, but don't do a breaking change because you believe nothing in your code has changed.

However, now its impossible for cargo to unify your dependency on bar with foo's dependency on bar, because they're different major versions. This means that you::Bar and foo::Bar are no longer the same type. If I upgrade my dependency on you, I will get a breakage, so you have made a breaking change.

This comment has been minimized.

@untitaker

untitaker Apr 19, 2017

Contributor

Good point, I completely overlooked that aspect. I have a few other concerns/questions about this feature (that don't really affect this PR), but let's cut this discussion short to not clutter this PR.

@untitaker

untitaker Apr 19, 2017

Contributor

Good point, I completely overlooked that aspect. I have a few other concerns/questions about this feature (that don't really affect this PR), but let's cut this discussion short to not clutter this PR.

This comment has been minimized.

@mathstuf

mathstuf Apr 19, 2017

What if you relax the range on bar to include the new version rather than just bumping to only using the latest version. What kind of bump does that mandate?

ISTR seeing that it would be a major change due to cargo always choosing at the high end of the version constraint range. Would it be worth an RFC to cargo to have it see dependencies of >= 1.1, < 3 and ^1 in different crates to, instead of deciding on two copies, one of version 2.x and another of, say, 1.5, instead see that 1.5 satisfies both and ignore the 2.x version?

@mathstuf

mathstuf Apr 19, 2017

What if you relax the range on bar to include the new version rather than just bumping to only using the latest version. What kind of bump does that mandate?

ISTR seeing that it would be a major change due to cargo always choosing at the high end of the version constraint range. Would it be worth an RFC to cargo to have it see dependencies of >= 1.1, < 3 and ^1 in different crates to, instead of deciding on two copies, one of version 2.x and another of, say, 1.5, instead see that 1.5 satisfies both and ignore the 2.x version?

This comment has been minimized.

@untitaker

untitaker Apr 19, 2017

Contributor

@mathstuf yeah this is what I thought about as well but consider off-topic for this RFC

@untitaker

untitaker Apr 19, 2017

Contributor

@mathstuf yeah this is what I thought about as well but consider off-topic for this RFC

Show outdated Hide outdated text/0000-public-private-dependencies.md
A: Public dependencies are public within a reachable subgraph but can become private if a
crate stops exposing a public dependency. For instance it is very possible to have a
family of crates that all depend on a utility crate that provides common types which is
a public dependency for all of them. However your own crate only becomes a user of this

This comment has been minimized.

@untitaker

untitaker Apr 18, 2017

Contributor

This paragraph has confusing wording, at least to me. To reiterate the point of this example, you could say that "public" is a property of the dependency relation (the edge in the dependency graph) and not of the crate or the package (the node).

@untitaker

untitaker Apr 18, 2017

Contributor

This paragraph has confusing wording, at least to me. To reiterate the point of this example, you could say that "public" is a property of the dependency relation (the edge in the dependency graph) and not of the crate or the package (the node).

@mark-i-m

This comment has been minimized.

Show comment
Hide comment
@mark-i-m

mark-i-m Apr 19, 2017

Contributor

Question: if your crate depends on multiple versions of the same dependency, does the compiled binary just have multiple copies of the same generated code? How does this affect name resolution in the compiler? Or linking? Also, is the binary proportionally larger?

Contributor

mark-i-m commented Apr 19, 2017

Question: if your crate depends on multiple versions of the same dependency, does the compiled binary just have multiple copies of the same generated code? How does this affect name resolution in the compiler? Or linking? Also, is the binary proportionally larger?

@bbatha

This comment has been minimized.

Show comment
Hide comment
@bbatha

bbatha Apr 19, 2017

Public dependencies shouldn't just be restricted to public APIs. For instance, most -sys crates can only be used once due to C's linkage rules. Noting libc as a public dependency would have made may have limited the impact of the libc-ocolypse. It would also be convenient to have a notion of a "unique" crate for -sys crates and for #[allocator] crates but I think that feature is orthogonal, and complex enough that it should be left as a future extension.

bbatha commented Apr 19, 2017

Public dependencies shouldn't just be restricted to public APIs. For instance, most -sys crates can only be used once due to C's linkage rules. Noting libc as a public dependency would have made may have limited the impact of the libc-ocolypse. It would also be convenient to have a notion of a "unique" crate for -sys crates and for #[allocator] crates but I think that feature is orthogonal, and complex enough that it should be left as a future extension.

@sfackler

This comment has been minimized.

Show comment
Hide comment
@sfackler

sfackler Apr 19, 2017

Member

@bbatha libc does not have the single-version constraint. The libc-pocalypse pain was just the normal issue of crates trying to bridge libc 0.1 and 0.2 APIs.

There's already a notion of a "unique" -sys crate - setting the links field in Cargo.toml.

Member

sfackler commented Apr 19, 2017

@bbatha libc does not have the single-version constraint. The libc-pocalypse pain was just the normal issue of crates trying to bridge libc 0.1 and 0.2 APIs.

There's already a notion of a "unique" -sys crate - setting the links field in Cargo.toml.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Apr 19, 2017

Member

If dependencies default to private, and you can't export types from private dependencies, then this introduces a compatibility break.

Member

joshtriplett commented Apr 19, 2017

If dependencies default to private, and you can't export types from private dependencies, then this introduces a compatibility break.

@joshtriplett

This comment has been minimized.

Show comment
Hide comment
@joshtriplett

joshtriplett Apr 19, 2017

Member

(That said, I'd love to see a change like this.)

Member

joshtriplett commented Apr 19, 2017

(That said, I'd love to see a change like this.)

@mitsuhiko

This comment has been minimized.

Show comment
Hide comment
@mitsuhiko

mitsuhiko Apr 19, 2017

Contributor

@joshtriplett that's why initially it will only be a compiler/cargo publish warning.

Contributor

mitsuhiko commented Apr 19, 2017

@joshtriplett that's why initially it will only be a compiler/cargo publish warning.

@withoutboats withoutboats added the T-lang label Apr 19, 2017

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Apr 19, 2017

Contributor

I think this is a real problem and this seems like a good solution, but it is a pretty big change. Even though you're starting with a warning, everyone is going to get the warning & be compelled to fix them. This is a huge burst of churn & the drawbacks section should at least note this as a drawback.

Contributor

withoutboats commented Apr 19, 2017

I think this is a real problem and this seems like a good solution, but it is a pretty big change. Even though you're starting with a warning, everyone is going to get the warning & be compelled to fix them. This is a huge burst of churn & the drawbacks section should at least note this as a drawback.

Show outdated Hide outdated text/0000-public-private-dependencies.md
**Q: How does semver and depenencies interact?**<br>
A: It is already the case that changing your own dependencies would require a semver
bumb for your own library because your API contract to the outside world changes. This

This comment has been minimized.

@untitaker

untitaker Apr 19, 2017

Contributor

s/bumb/bump/

@untitaker

untitaker Apr 19, 2017

Contributor

s/bumb/bump/

Show outdated Hide outdated text/0000-public-private-dependencies.md
on the crate that actually implements that type. The limitations from the previous
answer apply (eg: you can currently overrule the restrictions).
**Q: How does semver and depenencies interact?**<br>

This comment has been minimized.

@untitaker

untitaker Apr 19, 2017

Contributor

s/depenencies/dependencies/

@untitaker

untitaker Apr 19, 2017

Contributor

s/depenencies/dependencies/

Show outdated Hide outdated text/0000-public-private-dependencies.md
```toml
[dependencies]
url = { version = "1.4.0", public = true }

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 20, 2017

Contributor

We also need to be able to express a private dependency which is locked with another dep's public dependency, see my running a -> c; a ->b; b -> c example from above.

@Ericson2314

Ericson2314 Apr 20, 2017

Contributor

We also need to be able to express a private dependency which is locked with another dep's public dependency, see my running a -> c; a ->b; b -> c example from above.

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 20, 2017

Contributor

@Ericson2314 is this really necessary? Why can you not just have a pin on the major version? eg: url = { version = "1", public = true }.

@mitsuhiko

mitsuhiko Apr 20, 2017

Contributor

@Ericson2314 is this really necessary? Why can you not just have a pin on the major version? eg: url = { version = "1", public = true }.

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

@mitsuhiko actually it might not be, but we should make not of this in the semantics: just as your public deps must be unique version per name, so your private dependencies and their public dependencies must be unique version per name.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

@mitsuhiko actually it might not be, but we should make not of this in the semantics: just as your public deps must be unique version per name, so your private dependencies and their public dependencies must be unique version per name.

This comment has been minimized.

@eternaleye

eternaleye Apr 21, 2017

I think this is one place where @bbatha's concern (link-only-once) diverges from what's described by (and very useful for) this RFC, and the two probably need handled distinctly.

In particular, in the absence of link-only-once dependencies, a private dependency hides all transitive dependencies behind it - and this is hugely useful in some cases.

However, link-only-once crates must not be hidden in this manner.

As a result, I think these might be better served by orthogonal mechanisms.

@eternaleye

eternaleye Apr 21, 2017

I think this is one place where @bbatha's concern (link-only-once) diverges from what's described by (and very useful for) this RFC, and the two probably need handled distinctly.

In particular, in the absence of link-only-once dependencies, a private dependency hides all transitive dependencies behind it - and this is hugely useful in some cases.

However, link-only-once crates must not be hidden in this manner.

As a result, I think these might be better served by orthogonal mechanisms.

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

Yeah. I am not entirely sure how to best deal with link only dependencies. They are definitely iffy and out of the scope as far as I'm concerned. I will add a section about those to the RFC.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

Yeah. I am not entirely sure how to best deal with link only dependencies. They are definitely iffy and out of the scope as far as I'm concerned. I will add a section about those to the RFC.

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

Agree that it's out of scope. Simply requiring that all link-once deps be reachable through chains of public dependencies is sound, but way overly restrictive.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

Agree that it's out of scope. Simply requiring that all link-once deps be reachable through chains of public dependencies is sound, but way overly restrictive.

Show outdated Hide outdated text/0000-public-private-dependencies.md
```toml
[dependencies]
url = { version = "1.4.0", public = true }

This comment has been minimized.

@mathstuf

mathstuf Apr 20, 2017

How about cratename = { from_dependencies = true } (or similar)? You're relying on deps to not break API with their deps being bumped, but that's true anyways already.

@mathstuf

mathstuf Apr 20, 2017

How about cratename = { from_dependencies = true } (or similar)? You're relying on deps to not break API with their deps being bumped, but that's true anyways already.

This comment has been minimized.

@untitaker

untitaker Apr 20, 2017

Contributor

I don't understand why from_dependencies is a better name than public, or how the version could be inferred. Could you give an example?

@untitaker

untitaker Apr 20, 2017

Contributor

I don't understand why from_dependencies is a better name than public, or how the version could be inferred. Could you give an example?

This comment has been minimized.

@mathstuf

mathstuf Apr 20, 2017

It'd be a replacement for version = "…" and an indication that cargo should figure out the version based on public dependencies of dependencies instead of trying to give a constraint directly. Basically "I'm fine with whatever my dependencies need because that's how I use this crate".

@mathstuf

mathstuf Apr 20, 2017

It'd be a replacement for version = "…" and an indication that cargo should figure out the version based on public dependencies of dependencies instead of trying to give a constraint directly. Basically "I'm fine with whatever my dependencies need because that's how I use this crate".

This comment has been minimized.

@untitaker

untitaker Apr 20, 2017

Contributor

I see, as far as I understand this could be a different RFC independent from this one though.

@untitaker

untitaker Apr 20, 2017

Contributor

I see, as far as I understand this could be a different RFC independent from this one though.

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

I think people should just pin less strictly instead. It's fine to just pin for 1 for instance.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

I think people should just pin less strictly instead. It's fine to just pin for 1 for instance.

This comment has been minimized.

@mathstuf

mathstuf Apr 21, 2017

But if I use a crate only because dependencies use it, why not have a way to say "I use it only because dependencies make me use it; let them tell me what version I need"?

In any case, probably a topic for a separate RFC.

@mathstuf

mathstuf Apr 21, 2017

But if I use a crate only because dependencies use it, why not have a way to say "I use it only because dependencies make me use it; let them tell me what version I need"?

In any case, probably a topic for a separate RFC.

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

But if I use a crate only because dependencies use it

If you only use it because dependencies use it and it's not part of their APIs the dependency is invisible to you. If it's a visible (public) dependency then the version of the dependency is relevant to you.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

But if I use a crate only because dependencies use it

If you only use it because dependencies use it and it's not part of their APIs the dependency is invisible to you. If it's a visible (public) dependency then the version of the dependency is relevant to you.

Show outdated Hide outdated text/0000-public-private-dependencies.md
solve some of these issues and also let us lift some restrictions that should make some
code compile that previously was prevented from compiling by restrictions in cargo.
**Q: What is a public dependency?**<br>

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 20, 2017

Contributor

I don't really like this language. If you depend on items from a dependency (e.g. types) even if you don't reexport them, you still need a public dep. Also----and this is crucial---if you don't reexport anything, it's not a breaking change to change your public deps if you're own interface doesn't change.

Consider a situation where a depends (privately or publicly) on b which publicly depends on c. To use any items from b with interfaces relying on c, a should also need to depend on c (and constrain that dep to be unified with b's). But now the extra dep of a on c forces the "effective interface: of b with given c to not break. Conversely, if a doesn't do that, then we know b's use of c is irrelevant so the relevant parts of b also won't break.

This is an instance of the general principle that the version of a crate is like a fallback / catchall that describes the remnants of the interface not already accounted for in the crate metadata, and constrainable with a dependency. Public deps are accounted for and can be constrained downstream, and thus need not cause interface breakage in and of themselves.

This may sounds like needlessly fancy reasoning, but this is actually really important practically. Breaking changes already are difficult growing pain with large ecosystems, and would become impossibly so if on every upstream breaking change, all publicly depending downstream was forced to issue their own breaking change even if all they did is bump a dependency. Conversely, tiny breaking changes are far easier to deal with if every publicly depending downstream libraries that aren't affected (i.e. most) just need to make a new release with a relaxed upper bound on their public dep).

@Ericson2314

Ericson2314 Apr 20, 2017

Contributor

I don't really like this language. If you depend on items from a dependency (e.g. types) even if you don't reexport them, you still need a public dep. Also----and this is crucial---if you don't reexport anything, it's not a breaking change to change your public deps if you're own interface doesn't change.

Consider a situation where a depends (privately or publicly) on b which publicly depends on c. To use any items from b with interfaces relying on c, a should also need to depend on c (and constrain that dep to be unified with b's). But now the extra dep of a on c forces the "effective interface: of b with given c to not break. Conversely, if a doesn't do that, then we know b's use of c is irrelevant so the relevant parts of b also won't break.

This is an instance of the general principle that the version of a crate is like a fallback / catchall that describes the remnants of the interface not already accounted for in the crate metadata, and constrainable with a dependency. Public deps are accounted for and can be constrained downstream, and thus need not cause interface breakage in and of themselves.

This may sounds like needlessly fancy reasoning, but this is actually really important practically. Breaking changes already are difficult growing pain with large ecosystems, and would become impossibly so if on every upstream breaking change, all publicly depending downstream was forced to issue their own breaking change even if all they did is bump a dependency. Conversely, tiny breaking changes are far easier to deal with if every publicly depending downstream libraries that aren't affected (i.e. most) just need to make a new release with a relaxed upper bound on their public dep).

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 20, 2017

Contributor

If you depend on items from a dependency (e.g. types) even if you don't reexport them, you still need a public dep.

Can you clarify what you mean by that? If you do not re-export them there is no need for a dependency to be public.

@mitsuhiko

mitsuhiko Apr 20, 2017

Contributor

If you depend on items from a dependency (e.g. types) even if you don't reexport them, you still need a public dep.

Can you clarify what you mean by that? If you do not re-export them there is no need for a dependency to be public.

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

Perhaps we are using "re-export" differently? To me, reexport means you include the *definition" in your interface, i.e. with a pub use, as opposely merely using the dependency's items. Reexporting is bad thing to do because then the public dep really does become part of your API in ways downstream may not control --- merely bumping a version bound over a breaking change may indeed be a breaking change.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

Perhaps we are using "re-export" differently? To me, reexport means you include the *definition" in your interface, i.e. with a pub use, as opposely merely using the dependency's items. Reexporting is bad thing to do because then the public dep really does become part of your API in ways downstream may not control --- merely bumping a version bound over a breaking change may indeed be a breaking change.

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

Correct. If you accept a type from a crate as parameter it means it's re-exported in your API.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

Correct. If you accept a type from a crate as parameter it means it's re-exported in your API.

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

Uh I am not sure which you are agreeing with?

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

Uh I am not sure which you are agreeing with?

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

Maybe I'm just to dumb to understand the comment but I do not quite follow what the difference here would be. Is the idea that if you use a subset of b that does not c you do not want a public dependency to c? That should be covered anyways.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

Maybe I'm just to dumb to understand the comment but I do not quite follow what the difference here would be. Is the idea that if you use a subset of b that does not c you do not want a public dependency to c? That should be covered anyways.

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

@mitsuhiko With the terminology change, the only remaining issue is the sentence: "Effectively the idea is that if your own library bumps a public dependency it means that it's a breaking change of your own crate."

I realize now this sentence is doesn't actually effect what this RFC specifies, so as far as the detailed design goes we're all good---feel free to just cut that sentance and ignore the rest of this post :).

But for the record, actually a changing of a dep should never be a breaking change. In the private dep case, downstream cannot tell at all, so we're good. In the public dep case, the solver will simply not use the new crate if the public dep cannot be unified with other public deps.

I wrote before

This is an instance of the general principle that the version of a crate is like a fallback / catchall that describes the remnants of the interface not already accounted for in the crate metadata, and constrainable with a dependency.

I think I have a better way of describing things. The general maxim for compat is "if I publish this crate, in all solutions where this this crate could be substituted for another, things must work in both case". If there is a solution where the substitution breaks, we could try to fix that case, or we could try to rule it out.

Since public deps may be exposed but also must exist once, any bounds adjustment is OK because the version unification would rule out laxer bounds switching to another version the other crates in the plan cannot cope with.

Mathematically, one can view a crate wrt compatability as Map<PubDeps, Map<Name, Item> (maps are partial functions). Just as adding definitions to a crate preserves compatibility, so relaxing bounds does too---both just extend domains of the partial functions. Whereas removing definitions does not preserves compatibility, tightening bounds is fine because solver-time failures are fine (the solver just moves on) whereas build-time errors aren't.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

@mitsuhiko With the terminology change, the only remaining issue is the sentence: "Effectively the idea is that if your own library bumps a public dependency it means that it's a breaking change of your own crate."

I realize now this sentence is doesn't actually effect what this RFC specifies, so as far as the detailed design goes we're all good---feel free to just cut that sentance and ignore the rest of this post :).

But for the record, actually a changing of a dep should never be a breaking change. In the private dep case, downstream cannot tell at all, so we're good. In the public dep case, the solver will simply not use the new crate if the public dep cannot be unified with other public deps.

I wrote before

This is an instance of the general principle that the version of a crate is like a fallback / catchall that describes the remnants of the interface not already accounted for in the crate metadata, and constrainable with a dependency.

I think I have a better way of describing things. The general maxim for compat is "if I publish this crate, in all solutions where this this crate could be substituted for another, things must work in both case". If there is a solution where the substitution breaks, we could try to fix that case, or we could try to rule it out.

Since public deps may be exposed but also must exist once, any bounds adjustment is OK because the version unification would rule out laxer bounds switching to another version the other crates in the plan cannot cope with.

Mathematically, one can view a crate wrt compatability as Map<PubDeps, Map<Name, Item> (maps are partial functions). Just as adding definitions to a crate preserves compatibility, so relaxing bounds does too---both just extend domains of the partial functions. Whereas removing definitions does not preserves compatibility, tightening bounds is fine because solver-time failures are fine (the solver just moves on) whereas build-time errors aren't.

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

Hmm @alexcrichton in #1977 (comment) you wrote

When libc reaches 1.0 then many crates will need to bump their major version as libc is a public dependency.

But per this thread it would only be a minor bump. Using this side-thread as it's not really a core part of the RFC but do want to get this clarified.

It's possible we'd need to tweak some things like method resolution for what I said to actually be true. (e.g. do the inherent methods of a reexported type "vagabound" with the type and are usuable via the export?)

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

Hmm @alexcrichton in #1977 (comment) you wrote

When libc reaches 1.0 then many crates will need to bump their major version as libc is a public dependency.

But per this thread it would only be a minor bump. Using this side-thread as it's not really a core part of the RFC but do want to get this clarified.

It's possible we'd need to tweak some things like method resolution for what I said to actually be true. (e.g. do the inherent methods of a reexported type "vagabound" with the type and are usuable via the export?)

This comment has been minimized.

@Ixrec

Ixrec Apr 22, 2017

Contributor

I believe Alex meant all the crates with a public dependency (direct or transitive) on libc would need a major version bump when libc goes 1.0, but all the crates with a private dependency (direct or transitive) on libc would not. That's why the next sentence of that comment is:

This RFC will allow us to precisely identify what set of crates need to be bumped, transitively!

Where I believe "need to be bumped" refers only to major version bumps.

@Ixrec

Ixrec Apr 22, 2017

Contributor

I believe Alex meant all the crates with a public dependency (direct or transitive) on libc would need a major version bump when libc goes 1.0, but all the crates with a private dependency (direct or transitive) on libc would not. That's why the next sentence of that comment is:

This RFC will allow us to precisely identify what set of crates need to be bumped, transitively!

Where I believe "need to be bumped" refers only to major version bumps.

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 22, 2017

Contributor

Right, I'm trying to argue that, absent reexports (as opposed to mere exposing of departure) changing dependencies in any way is not a breaking change because every plan the build could go wrong the solver will disallow anyways.

@Ericson2314

Ericson2314 Apr 22, 2017

Contributor

Right, I'm trying to argue that, absent reexports (as opposed to mere exposing of departure) changing dependencies in any way is not a breaking change because every plan the build could go wrong the solver will disallow anyways.

@kornelski

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Apr 20, 2017

Contributor

Is the crate using the dependency the right place to declare it?

It could be a declaration in the dependency itself, similar to the link attribute (i.e. the dependency would mark itself as intended to be shared and exist only once [within any subtree of dependencies?]).
IMHO that would be a significant improvement in usability, because it frees all users of the crate from understanding the concept and marking their dependencies appropriately.

Perhaps it could even be a declaration per type or module? (e.g. pub(globally unique) or pub(no re-export).) e.g. an Error type may be intended to be re-exported, but other types should not.

Contributor

kornelski commented Apr 20, 2017

Is the crate using the dependency the right place to declare it?

It could be a declaration in the dependency itself, similar to the link attribute (i.e. the dependency would mark itself as intended to be shared and exist only once [within any subtree of dependencies?]).
IMHO that would be a significant improvement in usability, because it frees all users of the crate from understanding the concept and marking their dependencies appropriately.

Perhaps it could even be a declaration per type or module? (e.g. pub(globally unique) or pub(no re-export).) e.g. an Error type may be intended to be re-exported, but other types should not.

@kornelski

This comment has been minimized.

Show comment
Hide comment
@kornelski

kornelski Apr 20, 2017

Contributor

Does it have to be explicit at all? Can cargo/rustc notice the situation and emit a warning? (and you could silence the warning with either allow attribute or some cargo.toml flag)

warning: you have two copies of foolib, because bar requires 1.x and baz requires 2.x

I'm assuming duplication of dependencies is generally unexpected (it's not a thing in C) and undesirable (it bloats executables), so users may want to avoid it even if it doesn't break any APIs.

Contributor

kornelski commented Apr 20, 2017

Does it have to be explicit at all? Can cargo/rustc notice the situation and emit a warning? (and you could silence the warning with either allow attribute or some cargo.toml flag)

warning: you have two copies of foolib, because bar requires 1.x and baz requires 2.x

I'm assuming duplication of dependencies is generally unexpected (it's not a thing in C) and undesirable (it bloats executables), so users may want to avoid it even if it doesn't break any APIs.

@untitaker

This comment has been minimized.

Show comment
Hide comment
@untitaker

untitaker Apr 20, 2017

Contributor

@pornel I think your proposal misses the point of this one. The point isn't to ensure that a dependency is used only once in the entire dependency graph, but to provide cargo with additional metadata that can then be used to potentially implement things like hinted at in the "Unresolved questions" section, or just improve the error messages that currently happen at version splits.

I'm assuming duplication of dependencies is generally unexpected

No, it's completely reasonable to have that for private dependencies.

You're right that this situation could potentially be detected by the compiler, however IMO this might turn out to be too complex (or slow for certain operations) to implement since now your dependency management (e.g. cargo update) depends on the sourcecode as well, not just the metadata.

Contributor

untitaker commented Apr 20, 2017

@pornel I think your proposal misses the point of this one. The point isn't to ensure that a dependency is used only once in the entire dependency graph, but to provide cargo with additional metadata that can then be used to potentially implement things like hinted at in the "Unresolved questions" section, or just improve the error messages that currently happen at version splits.

I'm assuming duplication of dependencies is generally unexpected

No, it's completely reasonable to have that for private dependencies.

You're right that this situation could potentially be detected by the compiler, however IMO this might turn out to be too complex (or slow for certain operations) to implement since now your dependency management (e.g. cargo update) depends on the sourcecode as well, not just the metadata.

@yodaldevoid

This comment has been minimized.

Show comment
Hide comment
@yodaldevoid

yodaldevoid Apr 20, 2017

Some related discussion: rust-lang/cargo#2064

Some related discussion: rust-lang/cargo#2064

Show outdated Hide outdated text/0000-public-private-dependencies.md
the compiler determined to be public but did not come from a public dependency. For
now it should be possible to publish anyways but in some period in the future it will
be necessary to explicitly mark all public dependencies as such or explicitly
mark them with `#[allow(external_private_dependency)]`.

This comment has been minimized.

@eternaleye

eternaleye Apr 20, 2017

I think you're missing something - specifically, I think this may require changes to the index format in order to allow Cargo to make dependency-resolution decisions based on whether dependencies are public.

Without changes to the index, either Cargo will still need to perform conservative "everything is public" resolution (and the situation does not actually improve), or it may perform "optimistic" resolution (as if everything is private) and decide on a resolution that it cannot tell is invalid until after it's downloaded the crate archives (which is wasteful, and offers no clean recovery path).

@eternaleye

eternaleye Apr 20, 2017

I think you're missing something - specifically, I think this may require changes to the index format in order to allow Cargo to make dependency-resolution decisions based on whether dependencies are public.

Without changes to the index, either Cargo will still need to perform conservative "everything is public" resolution (and the situation does not actually improve), or it may perform "optimistic" resolution (as if everything is private) and decide on a resolution that it cannot tell is invalid until after it's downloaded the crate archives (which is wasteful, and offers no clean recovery path).

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

@eternaleye why would cargo have to assume a public dependency?

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

@eternaleye why would cargo have to assume a public dependency?

This comment has been minimized.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

@mitsuhiko this is about whether the solver guess solutions which it then checks against dependency privacy, or whether it has access to privacy while solving---the latter is probably far more efficient as partial solutions can be ruled out earlier.

@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

@mitsuhiko this is about whether the solver guess solutions which it then checks against dependency privacy, or whether it has access to privacy while solving---the latter is probably far more efficient as partial solutions can be ruled out earlier.

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

@Ericson2314 cargo knows what dependencies are public from the definition in the Cargo.toml. The only thing the #[allow(...)] does is silence the warning (later error). I don't believe it needs to impact the dependency resolution algorithm but I might be missing something here.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

@Ericson2314 cargo knows what dependencies are public from the definition in the Cargo.toml. The only thing the #[allow(...)] does is silence the warning (later error). I don't believe it needs to impact the dependency resolution algorithm but I might be missing something here.

This comment has been minimized.

@eternaleye

eternaleye Apr 21, 2017

@mitsuhiko: The problem here is with the privacy/publicity of transitive dependencies. Imagine the following dependency graph:

  • A
    • B
      • D
      • E
    • C
      • E

All dependencies are public, and furthermore let us presume that these "E" dependencies are ones that, if one of them were private, would fall into resolutions that "previously [were] prevented from compiling by restrictions in cargo."

I clone A, and run cargo build. Cargo's dependency resolution does not have access to the Cargo.toml for anything other than A. For everything else, it accesses the index. As a result, in order to know whether B and C depend on E privately or publicly, the index must carry this information.

If it does not, Cargo has a few options:

  1. Presume they are all public, and reject the resolution. No improvement over today.
  2. Presume they are all public, and if the resolution would be rejected, fetch all potentially eligible crates in order to re-perform resolution using their Cargo.toml files. Incredibly network-intensive, may fail anyway, significant new logic.
  3. Presume they are all private, begin fetching the selected crates (which may have version skews that are not permissible!), and then discover then that the resolution is irresolvable. Network-intensive, offers no usable error handling path where some versions of B (or C) make E private and others make E public.
@eternaleye

eternaleye Apr 21, 2017

@mitsuhiko: The problem here is with the privacy/publicity of transitive dependencies. Imagine the following dependency graph:

  • A
    • B
      • D
      • E
    • C
      • E

All dependencies are public, and furthermore let us presume that these "E" dependencies are ones that, if one of them were private, would fall into resolutions that "previously [were] prevented from compiling by restrictions in cargo."

I clone A, and run cargo build. Cargo's dependency resolution does not have access to the Cargo.toml for anything other than A. For everything else, it accesses the index. As a result, in order to know whether B and C depend on E privately or publicly, the index must carry this information.

If it does not, Cargo has a few options:

  1. Presume they are all public, and reject the resolution. No improvement over today.
  2. Presume they are all public, and if the resolution would be rejected, fetch all potentially eligible crates in order to re-perform resolution using their Cargo.toml files. Incredibly network-intensive, may fail anyway, significant new logic.
  3. Presume they are all private, begin fetching the selected crates (which may have version skews that are not permissible!), and then discover then that the resolution is irresolvable. Network-intensive, offers no usable error handling path where some versions of B (or C) make E private and others make E public.

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

@eternaleye in case you are referring to the crates.io index, yes that information (public true/false) would be contained in the "deps" section.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

@eternaleye in case you are referring to the crates.io index, yes that information (public true/false) would be contained in the "deps" section.

This comment has been minimized.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

I for some reason thought you meant that #[allow(external_private_dependency)] needs to be reflected in the index.

@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

I for some reason thought you meant that #[allow(external_private_dependency)] needs to be reflected in the index.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Apr 21, 2017

Contributor

👍 to the general idea. Some random comments:

  • I think assuming private dependencies by default is an interesting choice: probably the right one, but not obviously so. I'd be curious to see some measurements on how often deps in practice are private vs public (I don't really know).
  • I'd like to hear a lot more details about what rules the compiler will use when issuing a warning.
  • This also seems related to the "private type in public API" rules (which have been a perennial point of controversy). I don't want to join the two topics together per se, but it seems to me that they are related in purpose. The existing rules aim to ensure that private structs are not exposed or usable outside your crate, and the same is true of the external deps.
Contributor

nikomatsakis commented Apr 21, 2017

👍 to the general idea. Some random comments:

  • I think assuming private dependencies by default is an interesting choice: probably the right one, but not obviously so. I'd be curious to see some measurements on how often deps in practice are private vs public (I don't really know).
  • I'd like to hear a lot more details about what rules the compiler will use when issuing a warning.
  • This also seems related to the "private type in public API" rules (which have been a perennial point of controversy). I don't want to join the two topics together per se, but it seems to me that they are related in purpose. The existing rules aim to ensure that private structs are not exposed or usable outside your crate, and the same is true of the external deps.
@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

I don't want to join the two topics together per se, but it seems to me that they are related in purpose.

I'm afraid we might. But maybe this isn't so bad. Comparability guarantees in conjunction wit this can be a clear and clearly-motivated way (even if those are only "guarantees" with warnings-as-errors) to evaluate those rules.

Contributor

Ericson2314 commented Apr 21, 2017

I don't want to join the two topics together per se, but it seems to me that they are related in purpose.

I'm afraid we might. But maybe this isn't so bad. Comparability guarantees in conjunction wit this can be a clear and clearly-motivated way (even if those are only "guarantees" with warnings-as-errors) to evaluate those rules.

@mitsuhiko

This comment has been minimized.

Show comment
Hide comment
@mitsuhiko

mitsuhiko Apr 21, 2017

Contributor

@mark-i-m i missed the question but the RFC changes nothing (so far) about how multiple dependencies functionally work in Rust. You can already end up with multiple versions of the same crate for as long as major versions are involved.

Contributor

mitsuhiko commented Apr 21, 2017

@mark-i-m i missed the question but the RFC changes nothing (so far) about how multiple dependencies functionally work in Rust. You can already end up with multiple versions of the same crate for as long as major versions are involved.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 21, 2017

Member

Thanks for writing up this RFC @mitsuhiko! Here's some thoughts I had while reading:

  • I believe @eternaleye is correct where the fact that a dependency is either public or private needs to be encoded into a registry's index. This is sort of like how the "kind" of dependency (build, dev, etc) is encoded today. This is I believe a very easy change, but is likely worth mentioning.
  • The RFC explicitly calls out how on a publish you'll get warnings, but this is happening on all rustc compilations, right? (that's what I'd like to happen, at least).
  • I, like @nikomatsakis, wouldn't mind seeing some more detail on the compiler side of things. Both in terms of what the argument looks like that Cargo is passing but also the way the compiler is detecting these apis.
  • The error messages here may run a risk of being confusing in some situations. For example let's say we've got A -> B -> C where B has a public dependency on C. This means that A can reexport some of C's types, but if you're a Cargo user then C could be way far down in the compilation graph and totally unknown. Do you think the compiler should go the extra mile and detect that C was exported transitively through B and tell A that B needs to be a public dependency? Put another way, if B is listed as a public dependency of A and then A reexported some of C's types, would the compiler warn/error about that?
  • Currently there's no mention of crate resolution in Cargo, but I find that to be a crucial aspect of public/private dependencies. Cargo will specifically at least need to start rejecting crate graph resolutions where two versions of a crate are publicly reachable to one another. This in turn would fix almost all of the woes brought up during rust-lang/cargo#2064 because Cargo will deterministically reach a statisfiable and compilable graph.

I think it's also worth mentioning in general some use cases of this RFC to mitigate pain seen in the ecosystem today. Note that this is mostly just an example, I'd encourage discussion of precisely what's happening here to happen elsewhere so this thread can continue to focus solely on this RFC. I consider public/private dependencies to be a big part of the answer to the major version upgrade of dependencies like libc. Once this change is implemented and propagated, I would feel confident about bumping the libc crate to 1.0.

When libc reaches 1.0 then many crates will need to bump their major version as libc is a public dependency. This RFC will allow us to precisely identify what set of crates need to be bumped, transitively! So immediately we'll have precise knowledge of how much of the ecosystem needs to be upgraded. After that Cargo will also reject any attempt to compile a graph which contains both libc 0.2 and libc 1.0 which would fail to compile. This is the crucial part of crate resolution I mentioned in the last bullet above. If Cargo determines that types from libc 0.2 can reach libc 1.0 then it will reject the resolution before we even start running the compiler. This means that error messages are immediate, high quality, and actionable.

Overall public/private dependencies empowers crate authors to release new major versions and:

  • Precisely know the impact of how many crates need to have a major version bump
  • Prevent any weird compiler error messages by having two versions of the library reach each other
  • Allow consumers to know exactly what crates need to be updated so they know where to file bugs and/or focus efforts

Note that public/private dependencies does not reduce the amount of work that needs to be done as part of a major version upgrade, it simply makes the experience of discovering the work before/after the major bump realistic. Similarly it enables authors to have precise knowledge of when they're done upgrading, because it's when Cargo starts to invoke the compiler!

Member

alexcrichton commented Apr 21, 2017

Thanks for writing up this RFC @mitsuhiko! Here's some thoughts I had while reading:

  • I believe @eternaleye is correct where the fact that a dependency is either public or private needs to be encoded into a registry's index. This is sort of like how the "kind" of dependency (build, dev, etc) is encoded today. This is I believe a very easy change, but is likely worth mentioning.
  • The RFC explicitly calls out how on a publish you'll get warnings, but this is happening on all rustc compilations, right? (that's what I'd like to happen, at least).
  • I, like @nikomatsakis, wouldn't mind seeing some more detail on the compiler side of things. Both in terms of what the argument looks like that Cargo is passing but also the way the compiler is detecting these apis.
  • The error messages here may run a risk of being confusing in some situations. For example let's say we've got A -> B -> C where B has a public dependency on C. This means that A can reexport some of C's types, but if you're a Cargo user then C could be way far down in the compilation graph and totally unknown. Do you think the compiler should go the extra mile and detect that C was exported transitively through B and tell A that B needs to be a public dependency? Put another way, if B is listed as a public dependency of A and then A reexported some of C's types, would the compiler warn/error about that?
  • Currently there's no mention of crate resolution in Cargo, but I find that to be a crucial aspect of public/private dependencies. Cargo will specifically at least need to start rejecting crate graph resolutions where two versions of a crate are publicly reachable to one another. This in turn would fix almost all of the woes brought up during rust-lang/cargo#2064 because Cargo will deterministically reach a statisfiable and compilable graph.

I think it's also worth mentioning in general some use cases of this RFC to mitigate pain seen in the ecosystem today. Note that this is mostly just an example, I'd encourage discussion of precisely what's happening here to happen elsewhere so this thread can continue to focus solely on this RFC. I consider public/private dependencies to be a big part of the answer to the major version upgrade of dependencies like libc. Once this change is implemented and propagated, I would feel confident about bumping the libc crate to 1.0.

When libc reaches 1.0 then many crates will need to bump their major version as libc is a public dependency. This RFC will allow us to precisely identify what set of crates need to be bumped, transitively! So immediately we'll have precise knowledge of how much of the ecosystem needs to be upgraded. After that Cargo will also reject any attempt to compile a graph which contains both libc 0.2 and libc 1.0 which would fail to compile. This is the crucial part of crate resolution I mentioned in the last bullet above. If Cargo determines that types from libc 0.2 can reach libc 1.0 then it will reject the resolution before we even start running the compiler. This means that error messages are immediate, high quality, and actionable.

Overall public/private dependencies empowers crate authors to release new major versions and:

  • Precisely know the impact of how many crates need to have a major version bump
  • Prevent any weird compiler error messages by having two versions of the library reach each other
  • Allow consumers to know exactly what crates need to be updated so they know where to file bugs and/or focus efforts

Note that public/private dependencies does not reduce the amount of work that needs to be done as part of a major version upgrade, it simply makes the experience of discovering the work before/after the major bump realistic. Similarly it enables authors to have precise knowledge of when they're done upgrading, because it's when Cargo starts to invoke the compiler!

@cuviper

This comment has been minimized.

Show comment
Hide comment
@cuviper

cuviper Apr 21, 2017

Member

Please remember to keep an eye on compatibility, especially if there are to be changes to the crates.io index.

Member

cuviper commented Apr 21, 2017

Please remember to keep an eye on compatibility, especially if there are to be changes to the crates.io index.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Apr 21, 2017

Contributor

@alexcrichton

  • The error messages here may run a risk of being confusing in some situations....

I'm very interested in this bullet, but having trouble following it.


edit I think I see all the points now.

In general I'd like to come up with a plan that prevents all such possible broken builds we care about, and then scale it back to something implementable. This would be opposed to figuring out something that seems feasible to do and then figuring out what build failures it prevents.

This stuff is already is quite hard in a simple language....and then throw in traits and my mind melts.

Contributor

Ericson2314 commented Apr 21, 2017

@alexcrichton

  • The error messages here may run a risk of being confusing in some situations....

I'm very interested in this bullet, but having trouble following it.


edit I think I see all the points now.

In general I'd like to come up with a plan that prevents all such possible broken builds we care about, and then scale it back to something implementable. This would be opposed to figuring out something that seems feasible to do and then figuring out what build failures it prevents.

This stuff is already is quite hard in a simple language....and then throw in traits and my mind melts.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Apr 24, 2017

Member

@Ericson2314

Oh let me clarify as well (even though it sounds like you've understood now).

Let's say we have three crates, A, B, and C. A depends on B, and B depends on C. We then have a few situations:

  • A reexports none of B, transitively reexporting none of C. In this case the compiler should issue no warnings.
  • A reexports some of B. If the compiler is informed that A publicly depends on B, then no warnings are issued. Otherwise, warnings are issued. In other words, this situation would require B to be tagged with public = true in A's Cargo.toml.
  • A reexports some of C's API. Because A doesn't depend on C this must transitively happen through B. In this example it means that B publicly depends on C. What happens in this case?
    • If the compiler is informed that A publicly depends on B, does the compiler warn? It seems like a simple implementation in the compiler would warn about this, but would end up having not very good ux.
    • Would the compiler require that it's informed of A publicly depending on C? This seems like it's difficult for Cargo to do so.

I was basically mostly wondering about the last case. How the compiler makes heads or tails of it and what error message it prints. If A didn't publicly depend on anything then I'd guess that a simple implementation would cause the compiler to warn "A needs to publicly depend on C". That's not quite accurate, though, as A needs to publicly depend on B, but the compiler also has to make headers or tails of that.

Member

alexcrichton commented Apr 24, 2017

@Ericson2314

Oh let me clarify as well (even though it sounds like you've understood now).

Let's say we have three crates, A, B, and C. A depends on B, and B depends on C. We then have a few situations:

  • A reexports none of B, transitively reexporting none of C. In this case the compiler should issue no warnings.
  • A reexports some of B. If the compiler is informed that A publicly depends on B, then no warnings are issued. Otherwise, warnings are issued. In other words, this situation would require B to be tagged with public = true in A's Cargo.toml.
  • A reexports some of C's API. Because A doesn't depend on C this must transitively happen through B. In this example it means that B publicly depends on C. What happens in this case?
    • If the compiler is informed that A publicly depends on B, does the compiler warn? It seems like a simple implementation in the compiler would warn about this, but would end up having not very good ux.
    • Would the compiler require that it's informed of A publicly depending on C? This seems like it's difficult for Cargo to do so.

I was basically mostly wondering about the last case. How the compiler makes heads or tails of it and what error message it prints. If A didn't publicly depend on anything then I'd guess that a simple implementation would cause the compiler to warn "A needs to publicly depend on C". That's not quite accurate, though, as A needs to publicly depend on B, but the compiler also has to make headers or tails of that.

@yodaldevoid

This comment has been minimized.

Show comment
Hide comment
@yodaldevoid

yodaldevoid Apr 24, 2017

@alexcrichton

As to the last case, would it be possible for the compiler to trace where the exported APIs are coming from? Thinking through it in my head, it seems to me that the compiler would have enough information to create a chain of re-exports/exports for each aspect of an API. If the compiler could figure out this chain, it would then be as simple as finding the first crate in the chain that is exporting an API (other than the crate being compiled, that is) and issuing a warning that states that that crate needs to be marked as a public dependency.

I suppose that an issue could arise if two dependencies re-export the same lower API, but in that case marking either, or both, as a public dependency should solve that problem. In fact, the right option may be to make both public dependencies, but I can't say I have thought very hard about this.

@alexcrichton

As to the last case, would it be possible for the compiler to trace where the exported APIs are coming from? Thinking through it in my head, it seems to me that the compiler would have enough information to create a chain of re-exports/exports for each aspect of an API. If the compiler could figure out this chain, it would then be as simple as finding the first crate in the chain that is exporting an API (other than the crate being compiled, that is) and issuing a warning that states that that crate needs to be marked as a public dependency.

I suppose that an issue could arise if two dependencies re-export the same lower API, but in that case marking either, or both, as a public dependency should solve that problem. In fact, the right option may be to make both public dependencies, but I can't say I have thought very hard about this.

Show outdated Hide outdated text/0000-public-private-dependencies.md
This will help the user understanding this concept and avoid making mistakes in
the `Cargo.toml`
Effectively the idea is that if your own library bumps a public dependency it means that

This comment has been minimized.

@Aaron1011

Aaron1011 Apr 25, 2017

Wouldn't this only be true for bumping the major version of a public dependency?

For example, say crate A version 1.0.0 depends on crate B version 2.0.0. Can A upgrade to B version 2.0.1 in a patch release (e.g. A version 1.0.1)?

@Aaron1011

Aaron1011 Apr 25, 2017

Wouldn't this only be true for bumping the major version of a public dependency?

For example, say crate A version 1.0.0 depends on crate B version 2.0.0. Can A upgrade to B version 2.0.1 in a patch release (e.g. A version 1.0.1)?

This comment has been minimized.

@jmesmon

jmesmon Apr 26, 2017

Presuming A's dependency on B is public, That depends on the version requirements that all packages which depend on A place on B, and the version requirements that all packages place on A.

As there are non-public crates (iow: ones that are not visible in the crates.io registry), we're limited in the assumptions that can be made about what version requirements are made.

@jmesmon

jmesmon Apr 26, 2017

Presuming A's dependency on B is public, That depends on the version requirements that all packages which depend on A place on B, and the version requirements that all packages place on A.

As there are non-public crates (iow: ones that are not visible in the crates.io registry), we're limited in the assumptions that can be made about what version requirements are made.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Apr 26, 2017

Contributor

@alexcrichton This relates to reexporting vs merely exposing.

On one hand, B's reexports are bonafide part of B's API, so no warning is even needed. A isn't doing anything wrong.

On the other hand, since B effective interface may magically depend on C, it's unsafe B to have anything but an exact-version dependency on C, because otherwise B is unable to uphold its compatibility guarantees even if C does:

Let <: be the crate interface compatibility relation. Let both crates "on their own" preserve comparability: C-1.0 <: C-1.1 and ∀c B-1.0(c) <: V-1.1(c), where Foo(Bar) is Foo instantiated with Bar. Those two alone don't mean B-1.0(C-1.1) <: B-1.1(C-1.0), yet both of those are valid instantiation assuming B has a normal C = "^1.0" dependency. A, even without a transitive reexport, can indeed rely on that last compatibility relationship holding.

While it is certainly is possible to transitively reexport, absent some typeof operator, it's impossible to transitively reexpose or reexport a mere exposition, which means A would not need to on things like B-1.0(C-1.1) <: B-1.1(C-1.0) when B only merely exposes C. This is why mere exposing is a far safer thing for crates to do, avoiding this quagmire.

[Note as each std is always tied to a single version of core, there are no potholes here for #1133.]

Contributor

Ericson2314 commented Apr 26, 2017

@alexcrichton This relates to reexporting vs merely exposing.

On one hand, B's reexports are bonafide part of B's API, so no warning is even needed. A isn't doing anything wrong.

On the other hand, since B effective interface may magically depend on C, it's unsafe B to have anything but an exact-version dependency on C, because otherwise B is unable to uphold its compatibility guarantees even if C does:

Let <: be the crate interface compatibility relation. Let both crates "on their own" preserve comparability: C-1.0 <: C-1.1 and ∀c B-1.0(c) <: V-1.1(c), where Foo(Bar) is Foo instantiated with Bar. Those two alone don't mean B-1.0(C-1.1) <: B-1.1(C-1.0), yet both of those are valid instantiation assuming B has a normal C = "^1.0" dependency. A, even without a transitive reexport, can indeed rely on that last compatibility relationship holding.

While it is certainly is possible to transitively reexport, absent some typeof operator, it's impossible to transitively reexpose or reexport a mere exposition, which means A would not need to on things like B-1.0(C-1.1) <: B-1.1(C-1.0) when B only merely exposes C. This is why mere exposing is a far safer thing for crates to do, avoiding this quagmire.

[Note as each std is always tied to a single version of core, there are no potholes here for #1133.]

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon May 3, 2017

Member

@alexcrichton @Ericson2314 This is a fascinating issue, thanks for digging into it!

I want to try to give a slightly more concrete version of (what I understand to be) @Ericson2314's concern. Suppose we have:

  • A depends on:
    • B 1.0 which publicly depends on C ^1.0
    • D 1.0, which has no dependencies
  • When A is initially built, it resolves to B 1.0 and C 1.1.
    • Because C's APIs are available to A via re-exports in B, A effectively depends on C 1.1 now, even though B only claims to depend on C ^1.0
    • In particular, the code in A might depend on APIs only available in C 1.1
    • However, if A is a library, we don't check in any lockfile for it, so this information is lost.
  • Now we change A to depend on D 1.1, which depends on C =1.0
    • A fresh copy of A, when built, will now resolve the crate graph to B 1.0, D 1.1, C 1.0
    • But now A may suddenly fail to compile, because it was implicitly depending on C 1.1 features via B.
Member

aturon commented May 3, 2017

@alexcrichton @Ericson2314 This is a fascinating issue, thanks for digging into it!

I want to try to give a slightly more concrete version of (what I understand to be) @Ericson2314's concern. Suppose we have:

  • A depends on:
    • B 1.0 which publicly depends on C ^1.0
    • D 1.0, which has no dependencies
  • When A is initially built, it resolves to B 1.0 and C 1.1.
    • Because C's APIs are available to A via re-exports in B, A effectively depends on C 1.1 now, even though B only claims to depend on C ^1.0
    • In particular, the code in A might depend on APIs only available in C 1.1
    • However, if A is a library, we don't check in any lockfile for it, so this information is lost.
  • Now we change A to depend on D 1.1, which depends on C =1.0
    • A fresh copy of A, when built, will now resolve the crate graph to B 1.0, D 1.1, C 1.0
    • But now A may suddenly fail to compile, because it was implicitly depending on C 1.1 features via B.
@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon May 3, 2017

Member

BTW, I think that the problem here is that D is not using a "semver-open" constraint (in the sense I spelled out here).

On the other hand, I think that if everyone is using semver-open constraints, given Cargo's greedy resolution strategy (which picks the biggest version possible), it should all work out.

And I think semver-open constraints are by far the most common. It'd be good to analyze how many crates in the ecosystem have non-semver-open constraints, and why.

Member

aturon commented May 3, 2017

BTW, I think that the problem here is that D is not using a "semver-open" constraint (in the sense I spelled out here).

On the other hand, I think that if everyone is using semver-open constraints, given Cargo's greedy resolution strategy (which picks the biggest version possible), it should all work out.

And I think semver-open constraints are by far the most common. It'd be good to analyze how many crates in the ecosystem have non-semver-open constraints, and why.

@cuviper

This comment has been minimized.

Show comment
Hide comment
@cuviper

cuviper May 3, 2017

Member

It'd be good to analyze how many crates in the ecosystem have non-semver-open constraints, and why.

Clap explicitly recommends a tilde constraint as a way of dealing with minimum-Rust compatibility.

Member

cuviper commented May 3, 2017

It'd be good to analyze how many crates in the ecosystem have non-semver-open constraints, and why.

Clap explicitly recommends a tilde constraint as a way of dealing with minimum-Rust compatibility.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton May 3, 2017

Member

Thanks for clarifying @aturon, but I think I may be failing to see how this is relevant to this RFC? I think it's highly relevant to #1890 in terms of defining what's a breaking change and decision on this RFC will affect the outcome of that RFC, but I'm not sure how it's directly related to public/private dependencies.

The semantics of crate resolution are not specified in this RFC so I'm also not sure if we're talking about a concrete proposal or not.

My current understanding of the relevance of this issue looks like:

  • Let's say that D privately depends on C. Cargo could allow this resolution or it could also not. The types from C 1.0 cannot reach C 1.1 because D does not leak types from C 1.0. Technically this can be allowed, but Cargo could also arbitrarily say that within one semver track (e.g. 1..) only one version is allowed (like it does today).
  • Let's say that D publicly depends on C. It seems in all possible versions of this RFC Cargo should reject such a resolution. It's possible for types from C 1.1 to reach C 1.0, an incompatibility, so Cargo would reject the resolution.

The only question here to me at least seems to be whether Cargo maximally allows duplication if it's technically feasible.

Member

alexcrichton commented May 3, 2017

Thanks for clarifying @aturon, but I think I may be failing to see how this is relevant to this RFC? I think it's highly relevant to #1890 in terms of defining what's a breaking change and decision on this RFC will affect the outcome of that RFC, but I'm not sure how it's directly related to public/private dependencies.

The semantics of crate resolution are not specified in this RFC so I'm also not sure if we're talking about a concrete proposal or not.

My current understanding of the relevance of this issue looks like:

  • Let's say that D privately depends on C. Cargo could allow this resolution or it could also not. The types from C 1.0 cannot reach C 1.1 because D does not leak types from C 1.0. Technically this can be allowed, but Cargo could also arbitrarily say that within one semver track (e.g. 1..) only one version is allowed (like it does today).
  • Let's say that D publicly depends on C. It seems in all possible versions of this RFC Cargo should reject such a resolution. It's possible for types from C 1.1 to reach C 1.0, an incompatibility, so Cargo would reject the resolution.

The only question here to me at least seems to be whether Cargo maximally allows duplication if it's technically feasible.

@mathstuf

This comment has been minimized.

Show comment
Hide comment
@mathstuf

mathstuf May 3, 2017

There are some packages which can't really follow semver. For example, the gitlab crate encodes the version of Gitlab that is supported (Gitlab does not provide stable hook entities[1] and can break on any given release). Rather than basically having a major version bump for any minor bump in Gitlab itself, instead, we just do 0.$gitlab.$patch and recommend an exact dependency on it since we can find "oops, that's actually nullable" at any time. What do we do for projects whose API is at the mercy of some external service that may not adhere to semver?

[1]I have issues open requesting a more stable API for hooks, but they're not prioritized.

mathstuf commented May 3, 2017

There are some packages which can't really follow semver. For example, the gitlab crate encodes the version of Gitlab that is supported (Gitlab does not provide stable hook entities[1] and can break on any given release). Rather than basically having a major version bump for any minor bump in Gitlab itself, instead, we just do 0.$gitlab.$patch and recommend an exact dependency on it since we can find "oops, that's actually nullable" at any time. What do we do for projects whose API is at the mercy of some external service that may not adhere to semver?

[1]I have issues open requesting a more stable API for hooks, but they're not prioritized.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 May 3, 2017

Contributor

@aturon it's not solved by assuming only semver-open. If B 1.0 depends on C >= 1, < 3 and D 1.1 depends on C ^1, A might accidentally depend on C ^2 and then fail with the D upgrade.

Contributor

Ericson2314 commented May 3, 2017

@aturon it's not solved by assuming only semver-open. If B 1.0 depends on C >= 1, < 3 and D 1.1 depends on C ^1, A might accidentally depend on C ^2 and then fail with the D upgrade.

@carols10cents

This comment has been minimized.

Show comment
Hide comment
@carols10cents

carols10cents Aug 29, 2017

Member

@withoutboats I have a question regarding this comment of yours:

@shepmaster You have to opt into the error by marking your dependency public. (Possibly there will be a warning encouraging you to do this.) But upgrading a warning to an error would not involve an opt-in.

I'm confused about whether we're talking about a compilation error or a version resolution error in this thread of conversation. Imagine this feature goes through being a cargo unstable feature and then becomes stabilized. What I think you're saying, if I'm an author of a library that is currently compiling and has a dependency used in my crate's public API, is that this will happen when I upgrade to the next stable Rust+Cargo and run cargo build:

  • I will get a warning saying that I'm using a dependency in my public API and that I should mark it as public.
  • If I mark that dependency as public, I'm "opting in to the error" as you said, but there is no error, because my crate was compiling before I upgraded Rust (so I must have had a valid resolution).
  • If I mark that dependency with #[allow(external_private_dependency)], the warning goes away? Or did we not want to allow allowing? Or only on crate publish did we want to disallow this, someday?

On the other hand, if my crate is NOT currently compiling because type url::Url expected, got type url::Url and I heard this new version of Rust would fix it, then what happens would be:

  • I will get a warning saying that I'm using a dependency in my public API and that I should mark it as public.
  • If I mark that dependency as public, I'm "opting in to the error". Next time I run cargo build, cargo will recognize that the graph in my Cargo.lock is now an invalid graph-- will it automatically throw that away and try looking for a valid graph given the current version constraints? And then it might error if there are no possible graphs? Or do I have to cargo update if I have a lockfile, to get cargo to retry resolution? I'm assuming that we mean a resolution error here, because otherwise I'd get the same compilation error as before upgrading Rust and we're trying to get rid of that.

I may have some misunderstandings in here, I would appreciate any corrections from anyone!

Member

carols10cents commented Aug 29, 2017

@withoutboats I have a question regarding this comment of yours:

@shepmaster You have to opt into the error by marking your dependency public. (Possibly there will be a warning encouraging you to do this.) But upgrading a warning to an error would not involve an opt-in.

I'm confused about whether we're talking about a compilation error or a version resolution error in this thread of conversation. Imagine this feature goes through being a cargo unstable feature and then becomes stabilized. What I think you're saying, if I'm an author of a library that is currently compiling and has a dependency used in my crate's public API, is that this will happen when I upgrade to the next stable Rust+Cargo and run cargo build:

  • I will get a warning saying that I'm using a dependency in my public API and that I should mark it as public.
  • If I mark that dependency as public, I'm "opting in to the error" as you said, but there is no error, because my crate was compiling before I upgraded Rust (so I must have had a valid resolution).
  • If I mark that dependency with #[allow(external_private_dependency)], the warning goes away? Or did we not want to allow allowing? Or only on crate publish did we want to disallow this, someday?

On the other hand, if my crate is NOT currently compiling because type url::Url expected, got type url::Url and I heard this new version of Rust would fix it, then what happens would be:

  • I will get a warning saying that I'm using a dependency in my public API and that I should mark it as public.
  • If I mark that dependency as public, I'm "opting in to the error". Next time I run cargo build, cargo will recognize that the graph in my Cargo.lock is now an invalid graph-- will it automatically throw that away and try looking for a valid graph given the current version constraints? And then it might error if there are no possible graphs? Or do I have to cargo update if I have a lockfile, to get cargo to retry resolution? I'm assuming that we mean a resolution error here, because otherwise I'd get the same compilation error as before upgrading Rust and we're trying to get rid of that.

I may have some misunderstandings in here, I would appreciate any corrections from anyone!

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Aug 29, 2017

Contributor

If I mark that dependency as public, I'm "opting in to the error" as you said, but there is no error, because my crate was compiling before I upgraded Rust (so I must have had a valid resolution).

The possibility is that there are errors in crates that depend on you, not errors in your crate, because they won't be able to construct a crate resolution graph if they also depend on a conflicting version of url or whatever.

This means marking a dependency public is a breaking change, so you need to release a new major version once you mark all your public dependencies correctly.

Contributor

withoutboats commented Aug 29, 2017

If I mark that dependency as public, I'm "opting in to the error" as you said, but there is no error, because my crate was compiling before I upgraded Rust (so I must have had a valid resolution).

The possibility is that there are errors in crates that depend on you, not errors in your crate, because they won't be able to construct a crate resolution graph if they also depend on a conflicting version of url or whatever.

This means marking a dependency public is a breaking change, so you need to release a new major version once you mark all your public dependencies correctly.

@carols10cents

This comment has been minimized.

Show comment
Hide comment
@carols10cents

carols10cents Aug 29, 2017

Member

The possibility is that there are errors in crates that depend on you, not errors in your crate, because they won't be able to construct a crate resolution graph if they also depend on a conflicting version of url or whatever.

This means marking a dependency public is a breaking change, so you need to release a new major version once you mark all your public dependencies correctly.

Ok. I think I get it. I was interpreting "i opt in to the error" as "it is now possible for me to get an error", but it's more like "i am opting in to bumping my major version and potentially causing errors for others". Is that right?

Everyone: I've made some changes to hopefully address the concerns. Please let me know what you think!

Member

carols10cents commented Aug 29, 2017

The possibility is that there are errors in crates that depend on you, not errors in your crate, because they won't be able to construct a crate resolution graph if they also depend on a conflicting version of url or whatever.

This means marking a dependency public is a breaking change, so you need to release a new major version once you mark all your public dependencies correctly.

Ok. I think I get it. I was interpreting "i opt in to the error" as "it is now possible for me to get an error", but it's more like "i am opting in to bumping my major version and potentially causing errors for others". Is that right?

Everyone: I've made some changes to hopefully address the concerns. Please let me know what you think!

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Aug 29, 2017

Contributor

Is that right?

Yep! And other people opt into possibly getting errors by upgrading to the new version of your crate.

Contributor

withoutboats commented Aug 29, 2017

Is that right?

Yep! And other people opt into possibly getting errors by upgrading to the new version of your crate.

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 1, 2017

Member

@rfcbot resolved publish-validate-lowest

Changes look great!

Member

alexcrichton commented Sep 1, 2017

@rfcbot resolved publish-validate-lowest

Changes look great!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 1, 2017

Member

@rfcbot resolution

Changes look great!

Member

alexcrichton commented Sep 1, 2017

@rfcbot resolution

Changes look great!

@alexcrichton

This comment has been minimized.

Show comment
Hide comment
@alexcrichton

alexcrichton Sep 1, 2017

Member

@rfcbot resolved resolution

Member

alexcrichton commented Sep 1, 2017

@rfcbot resolved resolution

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 7, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Sep 7, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Sep 14, 2017

Contributor

This basically means that if you privately depend on Hyper 0.3 and Hyper 0.4, that's an error.

Does that mean that it is impossible for a given type to implement serde::Serialize from both serde 0.9.x and serde 1.x.y? (Either through reexports like https://github.com/serde-rs/legacy, or with a future Cargo feature to locally rename dependent crates.)

That sounds bad :/

Contributor

SimonSapin commented Sep 14, 2017

This basically means that if you privately depend on Hyper 0.3 and Hyper 0.4, that's an error.

Does that mean that it is impossible for a given type to implement serde::Serialize from both serde 0.9.x and serde 1.x.y? (Either through reexports like https://github.com/serde-rs/legacy, or with a future Cargo feature to locally rename dependent crates.)

That sounds bad :/

@mathstuf

This comment has been minimized.

Show comment
Hide comment
@mathstuf

mathstuf Sep 14, 2017

Maybe if you have to go through some kind of "legacy" crate meant for bridging support for multiple versions of a single crate? There could be a field for that.

Though you still would have the problem that serde_derive can only generate for a single version of serde at a time…

Maybe if you have to go through some kind of "legacy" crate meant for bridging support for multiple versions of a single crate? There could be a field for that.

Though you still would have the problem that serde_derive can only generate for a single version of serde at a time…

@nox

This comment has been minimized.

Show comment
Hide comment
@nox

nox Sep 14, 2017

Contributor

Why distinguish public and private dependencies if it means you can't depend on different versions of a private dependency?

Contributor

nox commented Sep 14, 2017

Why distinguish public and private dependencies if it means you can't depend on different versions of a private dependency?

@yodaldevoid

This comment has been minimized.

Show comment
Hide comment
@yodaldevoid

yodaldevoid Sep 14, 2017

This basically means that if you privately depend on Hyper 0.3 and Hyper 0.4, that's an error.

This feels like an error or a case of a bad explanation. If this is true as I am reading this it would be a major change in behavior when it comes to crate version resolution that I see breaking many crates. Can we get some clarification on this?

yodaldevoid commented Sep 14, 2017

This basically means that if you privately depend on Hyper 0.3 and Hyper 0.4, that's an error.

This feels like an error or a case of a bad explanation. If this is true as I am reading this it would be a major change in behavior when it comes to crate version resolution that I see breaking many crates. Can we get some clarification on this?

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Sep 14, 2017

Contributor

@alexcrichton's comment I think needs more context. What he meant was that if two versions of hyper are "exposed" to you, its an error. A dependency is exposed to you if:

  • It is a direct dependency of your crate, public or private.
  • It is a public dependency of a direct dependency or another public dependency of a direct or public dependency (recursively into infinite).

That is, this error is exactly what the RFC is supposed to introduce. It won't break any existing code, because dependencies are marked private by default and you can't directly depend on the same crate twice (at least not in a way that would trigger this error).

Obviously, there are use cases for "version straddling" - like implementing Serialize from both versions of serde. This needs to be supported somehow, possibly involving an opt out of this error. If its not adequately resolved in the RFC text I would say we make it an unresolved question for now.

Contributor

withoutboats commented Sep 14, 2017

@alexcrichton's comment I think needs more context. What he meant was that if two versions of hyper are "exposed" to you, its an error. A dependency is exposed to you if:

  • It is a direct dependency of your crate, public or private.
  • It is a public dependency of a direct dependency or another public dependency of a direct or public dependency (recursively into infinite).

That is, this error is exactly what the RFC is supposed to introduce. It won't break any existing code, because dependencies are marked private by default and you can't directly depend on the same crate twice (at least not in a way that would trigger this error).

Obviously, there are use cases for "version straddling" - like implementing Serialize from both versions of serde. This needs to be supported somehow, possibly involving an opt out of this error. If its not adequately resolved in the RFC text I would say we make it an unresolved question for now.

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster Sep 14, 2017

Member

if two versions of hyper are "exposed" to you, its an error

In @SimonSapin's examples

reexports like https://github.com/serde-rs/legacy

Legacy would treat serde 0.9 as a private dependency, so all's well here.

a future Cargo feature to locally rename dependent crates

Would depend on how it's implemented, of course, but I imagine that it could work in a similar way. The important thing would be that the renamed crate couldn't have any public dependencies of its own that then conflict. For example, if crate A v1.0 has a public dependency on Common v1.0 and A v2.0 has a public dependency on Common v2.0, then I'd hope the warning / error would be raised (unless there's something that also renames the public dependencies...)

Member

shepmaster commented Sep 14, 2017

if two versions of hyper are "exposed" to you, its an error

In @SimonSapin's examples

reexports like https://github.com/serde-rs/legacy

Legacy would treat serde 0.9 as a private dependency, so all's well here.

a future Cargo feature to locally rename dependent crates

Would depend on how it's implemented, of course, but I imagine that it could work in a similar way. The important thing would be that the renamed crate couldn't have any public dependencies of its own that then conflict. For example, if crate A v1.0 has a public dependency on Common v1.0 and A v2.0 has a public dependency on Common v2.0, then I'd hope the warning / error would be raised (unless there's something that also renames the public dependencies...)

@SimonSapin

This comment has been minimized.

Show comment
Hide comment
@SimonSapin

SimonSapin Sep 14, 2017

Contributor

Legacy would treat serde 0.9 as a private dependency, so all's well here.

So implementing serde09::Serialize for a public type does not make serde09 a public dependency?

Contributor

SimonSapin commented Sep 14, 2017

Legacy would treat serde 0.9 as a private dependency, so all's well here.

So implementing serde09::Serialize for a public type does not make serde09 a public dependency?

@shepmaster

This comment has been minimized.

Show comment
Hide comment
@shepmaster

shepmaster Sep 14, 2017

Member

So implementing serde09::Serialize for a public type does not make serde09 a public dependency?

I believe it would make the crate serde09 (version 0.9) a public dependency, but it would not make the crate serde (version 0.9) a public dependency.

This assumes that the crate performing the implementation of serde09::Serialize has chosen to mark serde09 as a public dependency (as the default is to act as a private dependency).

Member

shepmaster commented Sep 14, 2017

So implementing serde09::Serialize for a public type does not make serde09 a public dependency?

I believe it would make the crate serde09 (version 0.9) a public dependency, but it would not make the crate serde (version 0.9) a public dependency.

This assumes that the crate performing the implementation of serde09::Serialize has chosen to mark serde09 as a public dependency (as the default is to act as a private dependency).

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Sep 14, 2017

Contributor

The real question is whether or not its sufficient for serde09 to just not marke serde a public dependency, or if we need some other mechanism here. I'm really unsure!

Contributor

withoutboats commented Sep 14, 2017

The real question is whether or not its sufficient for serde09 to just not marke serde a public dependency, or if we need some other mechanism here. I'm really unsure!

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 17, 2017

The final comment period is now complete.

rfcbot commented Sep 17, 2017

The final comment period is now complete.

@withoutboats withoutboats merged commit ff41075 into rust-lang:master Sep 17, 2017

@withoutboats

This comment has been minimized.

Show comment
Hide comment
@withoutboats

withoutboats Sep 17, 2017

Contributor

Merged this PR! Tracking issue is rust-lang/rust#44663. Thanks so much @mitsuhiko, @carols10cents, and everyone else for working on this RFC.

Excited to try to get this in nightly during the upcoming impl period. 🎉

Contributor

withoutboats commented Sep 17, 2017

Merged this PR! Tracking issue is rust-lang/rust#44663. Thanks so much @mitsuhiko, @carols10cents, and everyone else for working on this RFC.

Excited to try to get this in nightly during the upcoming impl period. 🎉

@alekratz

This comment has been minimized.

Show comment
Hide comment
@alekratz

alekratz Sep 21, 2017

The rendered RFC link gives a 404 - could it be updated to the current repository's copy please?

The rendered RFC link gives a 404 - could it be updated to the current repository's copy please?

@carols10cents

This comment has been minimized.

Show comment
Hide comment
@carols10cents

carols10cents Sep 21, 2017

Member

@alekratz

The rendered RFC link gives a 404 - could it be updated to the current repository's copy please?

Done!

Member

carols10cents commented Sep 21, 2017

@alekratz

The rendered RFC link gives a 404 - could it be updated to the current repository's copy please?

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment