Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transitive post dependencies can be unreliable #5010

Open
kit-ty-kate opened this issue Jan 19, 2022 · 4 comments
Open

Transitive post dependencies can be unreliable #5010

kit-ty-kate opened this issue Jan 19, 2022 · 4 comments

Comments

@kit-ty-kate
Copy link
Member

kit-ty-kate commented Jan 19, 2022

Let’s define

  • a package A with: depopt: ["B"]
  • a package B with: depends: ["A" {post}]
  • a package C with: depends: ["B"]

e.g. A is ctypes, B is ctypes-foreign and C is a user of ctypes

Depending on how C is installed, a race condition can occure if C uses A under the hood.
A can either be installed before (will succeed) or after (will fail) C or even cause race-condition where A is in a half-installed state.
The issue is that it is not obvious to the user and very tricky to debug for repository maintainers.

Example of such issue: ocaml/opam-repository@4960621

@dra27
Copy link
Member

dra27 commented Feb 4, 2022

Possible options discussed:

  • Considered whether we could automatically promote the post dependencies of your dependencies to being direct dependencies (this would automatically add A as a dependency of C) but there is a problem that this can introduce cycles. This could be done where cycle-introducing dependencies are avoided but it's more complicated and might lead to different unpredictability
  • Considered whether there could be a different flag or mechanisms in B which indicates that dependencies on it need to add other dependencies.

@gasche
Copy link
Member

gasche commented Feb 24, 2022

Currently, B having "A {post}" means "A can be installed at any point (before or) after B". In particular, other packages depending on B may be installed after B may before A, or concurrently with A. If they need A to be installed before, they should explicitly mark A as a dependency.

The problem that you report is that this UI is prone to misinterpretation. Users interpret "A {post}" as "A will be installed right after this package", because this is what they observe by running individual "opam install" commands. So they think their package only only needs to depends on B, even when they in fact depend on A. If you decide to automatically make A a normal dependency of any C depending on B, this is the behavior you are enforcing: from the point of view of packages depending on B, A gets installed "right after B".

But then @dra27 remarks that this can introduce cycles. If A decides to depend on another package D that itself depends on B, then we need to install (B; D; A), and not install A "right after B".

The following interpretation could solve this problem:

  • if C depends on B, and B post-depends on A, then by default C depends (rather than post-depends) on A
  • but if C explicitly post-depends on A, then we respect this explicit choice

Both interpretations (the current one and the proposed one) make sense / are consistent, the question is which one will make repository maintenance easier. I tend to think that my proposal is better than the current behavior:

  • This (B; D; A) situation only desired in unusual corner cases, while the (B; A; C) situation is the desired outcome in the common case. In the path of least effort when users of C/D don't say anything about A, the current status breaks the common case, while my proposal only breaks the corner case.
  • With my proposal, breakage shows up as a cycle introduced in the repository. I suspect that this could be easier to triage than a build failure as currently. (It's a less common error, so looking for that would led to this weird possibility quicker.)

The proposal does have one bad property, which is that it breaks the usual interpretation that adding dependencies only adds more constraints. Here adding a dependency can relax an existing inferred constraint. This is odd and possibly a reason to reject it.
(Note: it is not the case that, with this proposal, we have the interpret the conjunction of "normal dependency" and "post dependency" as "post dependency": if a package P depends on B1, B2, and B1 post-depends on A and B2 normal-depends on A, then P should normal-depend on A, not only post-depend on A. The "relaxation" only comes from the packaging instruction explicitly placed in P, never from the conjuction of the dependencies constraints)

@gasche
Copy link
Member

gasche commented Feb 24, 2022

Another problem with my proposal: in the situation where B post-depends on A, and we add a dependency D on A that depends on B (so we want the outcome (B; D; A)), things break with my proposal and they are fixed by changing the metadata of D, not A. We edit D to clarify that the dependency (which was assumed to be normal-depend) can in fact be relaxed. It is bad that a change in A's metadata requires a change in D.

The justification is that if D depends on B and can have A as a post-dependency and didn't say so explicitly, then (under the proposed bhavior) its packaging information is "too strict" and needs to be fixed to be made more accurate.

In a sense we are deciding who should be extra careful when they depend on B who post-depends on A:

  • people that wanted to depend on A as well, and should be careful to add the dependency epxlicitly (the current setting)
  • or people that only post-depend on A, and may need to be careful to relax the dependency explicitly (with the proposal)

@gasche
Copy link
Member

gasche commented Feb 24, 2022

Note: in the case of ctypes, a solution could be to split things as follows:

  • have a conf-ctypes-foreign package that checks if ctypes.foreign should be built, with ctypes as a post-dependency
  • ctypes optionally depends on conf-ctypes-foreign
  • have another ctypes-foreign that depends on ctypes and conf-ctypes-foreign, and instruct users to depend on that one

Then we get the expected behavior by default, without having to change OPAM's behavior in subtle way.

(Or: ctypes could be packaged differently to have just two packages, ctypes which does not try to build ctypes.foreign and and ctypes-foreign which depends on ctypes and does build ctypes.foreign unconditionally.)

Maybe the simplest solution is just to discourage the use of {post} dependencies for non-base packages!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants