ocaml / opam-repository

Main public package repository for opam, the source package manager of OCaml.
https://opam.ocaml.org
Creative Commons Zero v1.0 Universal
518 stars 1.14k forks source link

discourage `depext` outside `conf-*` packages #5791

Closed gasche closed 7 years ago

gasche commented 8 years ago

Hi,

Thinking about #5766 and #5783, it seems to me that it was a mistake to sprinkle depext dependencies in packages in the first place. Every replacement of a depext by a dependency on a conf-* package is a win, because it refers to a single point of definition for a given logical notion (system dependency on X), that can be updated to support other systems (instead of having to update each field using the depext(s)).

Are there cases where having a depext in a package is appropriate, and moving that to a conf-* package would be the wrong thing?

If there is not, I think we could consider doing the following to improve the quality of repositories:

  1. Teach opam lint to complain about depext field in packages whose name does not match conf-*.
  2. In a later version of OPAM, add a boolean external field (default: false), and only accept depext in packages with external: true.
samoht commented 8 years ago

That was the initial idea about conf-* packages, so I'm all in for that :-) As usual, we introduced the idea without enforcing it initially, but as it seems to be useful it'd be interesting to indeed get tooling support for that.

dsheets commented 8 years ago

What about putting a flag in the repo metadata that indicates under what conditions a package can have depexts? This would let non-ocaml/opam-repository users do what they want without introducing name-dependence or new package-level fields. A package-level field may also be a good idea, though.

yallop commented 8 years ago

Restricting depexts to distinguished packages is a good idea.

I don't think there's a need for a new flag, though, or for rules about package names. A diagnostic (from opam lint or otherwise) for packages that have both a url file and a depexts field should be sufficient. None of the conf- packages in the current repository have url files.

timbertson commented 8 years ago

I'm definitely all for this :+1: :)

gasche commented 8 years ago

@yallop I find your proposal a bit unsatisfying on a semantics level (there is nothing really logical about linking depexts and url in this way, and I'm not sure whether it makes sense to assume that conf packages cannot be downloaded). If this is a proposal meant to reduce the development burden, then I would rather use the conf-* restriction which is hackish but more meaningful.

@dsheets are there reason to suspect that not factoring all depexts in configuration packages is a good move for any non-ocaml or non-opam-repository repo? My intuition rather says that it would be a universally desirable thing. Of course, it should also be handled as a transition period requiring work of the repo maintainers -- only a mild opam lint mention at first, and after some time a firmer requirement. Or are you worried that adding a new per-package field could break some existing tooling for other repositories?

The conf-* naming convention may not generalize well to other repositories, though.

yallop commented 8 years ago

@gasche: the idea is that a package consists either of code to be built, accessible via a url, or of a set of depexts. In practice that's generally exactly what happens, and in the cases where it doesn't, people complain (see #5791).

dsheets commented 8 years ago

I don't like solutions based on names because suddenly names gain semantics when they should be opaque. I favor opting into this constraint because it is currently the case that you can easily write some bindings or other system-dependent code and throw depexts into your opam file. This works today and is very straightforward. For publication, I can definitely see the benefit to requiring depexts separated but there are opam tool and non-central repository users that simply don't care about that separation. This is why I support making Jeremy's condition optional at the repo level in the repo metadata file.

gasche commented 8 years ago

@dsheets the point of "opam lint" is to change the path of least resistance to steer towards people good packaging practice. I know that it is convenient for peole today to throw depexts in a package, but then it creates lots of problems down the road. My proposal is precisely to make this bad habit inconvenient by having Q/A tools complain about it, making users prefer other options. I'm all for minimizing the cost on existing package bases, but I do not believe that disabling this check for future package submissions is in the interest of any non-central repository, whether they care or not. I would thus rather be in favor of not letting people opt out for the indefinite future.

I would think about it this way: if OPAM was not released yet and we were discussing the semantics of the depexts feature, would you still argue in favor of letting people opt out of the restriction (let's say the package-level field option, or Jeremy's proposal) I propose in the indefinite future? If not, then we are discussing a transition period.

dsheets commented 8 years ago

@gasche Yes, I would argue in favor of letting people opt out of the restriction just as I am in favor of having optional warnings in the OCaml compiler. There should be only a single type of package definition and it currently makes sense (but isn't a particularly good idea) for a package to have both depexts and url. It is a convention and best practice to separate these notions but it isn't a requirement in general. In particular, with conf-* packages, there is still the possibility for repositories to diverge for similar/the same system package.

How would you implement the warning? In opam lint alone? I think opam publish and Camelus are the right places and, in those cases, the repository in question will be known. Generally, I favor putting more repository policy in the repo metadata file of repositories. I also think that a lot of the opam depext tags/flags should be available in the entirety of the opam file in order to do things like conditionalize build recipes on OS X package managers.

lefessan commented 8 years ago

Names are important for users. Deciding that depexts should only be stored in packages named conf-* is both easy to understand by them and easy to enforce by opam-lint.

The dedicated header solution does not make sense, since external: true would be found everywhere where a depexts is present, so it is just equivalent to putting a depexts. Having a complex link between the absence of url and the presence of depexts is too restrictive : some conf-* might want to use a standard feature detection package, that would have to be downloaded from the Internet.

Note that a standard behavior to come is to look for the package with the depexts for the library you need. Only the conf-* solution provides an easy way to achieve this task.

avsm commented 8 years ago
AltGr commented 8 years ago

Although the two obviously interfere, I like to keep the rules required by opam and repository convention separate when possible. conf-*, for example, is a naming convention, and I don't think it has its place in the runtime, while I am all for it on the repository, where naming conventions are very important. A virtual flag, on the other hand (or an external: true field, it's equivalent, but I tend to prefer a flag as it's more consistent with the current file definitions), would make perfect sense and fit better in the runtime.

Then, the repository convention would impose that virtual and conf-* are linked. A virtual package shouldn't have install/remove instructions, I didn't actually think about url, it would make sense to also forbid it, but I think it is a detail.

Note that opam lint (or @camelus, or opam publish, the linting code is shared¹) have numbered warnings/errors, so that they can be selectively disabled: it's advisory, the rules are, once again, imposed by the repository maintainers. An error in opam lint will never prevent you from using the package locally or on a private repo. On the other hand, the runtime would enforce the constraint when the virtual flag is present. We could also imagine having warnings that are not enabled by default be checked by @camelus...

¹ if it doesn't seem so it's because the tools aren't on the same version of the opam lib

dbuenzli commented 7 years ago

Just as a little stat the current number of packages that still have depexts in one of their versions in the current state of the repository is the following:

git grep depexts | grep -v "packages/conf" | cut -d / -f 2 | sort | uniq | wc -l 
     193
avsm commented 7 years ago

I think that any enforcement of depext requires better integration with the solver to account for the finer grained constraints needed (e.g. by OS version). The relevant opam issue is https://github.com/ocaml/opam/issues/2919

Until then, I can't see much progressing here in the opam-repo and don't see much point shuffling depexts around into packages until the broader feature is designed and implemented in opam2. I propose closing this issue and shifting the discussion to how best to solve it using the opam2 feature set instead.

gasche commented 7 years ago

The value of shuffling depexts around is that it allows to share a set of depexts across several packages (that depend on the same system dependency) or several versions of the same package (otherwise maintainers would only update depexts on recent versions, leaving users of, say, older OCaml versions in the dark). Does this not already make sense within the current opam language and feature set?

dbuenzli commented 7 years ago

I think that as @gasche mentions there's no harm for packages to gradually move to conf-* packages and try to ask package maintainers to use them. It may facilitate the transition.

However I don't think its worth keeping this issue open and discussion should indeed occur in https://github.com/ocaml/opam/issues/2919 so I'm closing this.