opam icon indicating copy to clipboard operation
opam copied to clipboard

[WIP] Move most of the external tools calls to a single module

Open kit-ty-kate opened this issue 7 years ago • 2 comments

This PR aims at improving the way of describing and looking for external programs calls. This should also ease portability checks for Windows (cc @dra27) and minimal distributions such as Alpine Linux for example by checking which packages to install, what options are not available on the distribution/OS you want to port opam on.

Related to #3197

This is WIP because of some assert false to fill up in src/format/opamFile.ml and calls to OpamExternalTools.custom for Git/Hg/Darcs/… Also I would know if the change to the API is ok before continuing further.

kit-ty-kate avatar Feb 09 '18 17:02 kit-ty-kate

Thanks. I would have liked to discuss this beforehand, but I think it's a good idea.

A few comments:

  • please update doc/index.html with the new module
  • the name OpamExternalTools fits the contents of the module well. However, it feels weird that e.g. OpamSystem.make_command now relies on OpamExternalTools.t (and OpamExternalTools.custom) when its principal use is to call the commands from package scripts (e.g. the contents of the build: and install: fields, see OpamAction.make_command). I would also prefer that OpamProcess wouldn't depend on the new module at all, since it doesn't rely on external commands (it'd be nice to avoid the dependency for OpamStd, too, but I don't see how to achieve that easily).
  • maybe there could be a distinction between tools that opam relies on (cp, mv, tar, patch, git, etc.), and the ones that are just polled at some point (for example, while I think this is generally a great improvement in readability, in the specific case of OpamSysPoll, where commands are just checked once, and are not always expected to exist, the benefit is not clear. It's no longer obvious that getprop is only ever useful to detect an Android device, for example)

These are details, but gathering the commands like this is a good thing and changing from a list to a cmd, args pair is also a good change (that I am sure you noticed had been done for the low-level command engine, but without reaching most calling points).

One last thing, if you find the combination of OpamSystem and OpamFilename awkward, I totally agree... this might be the last part I didn't get around to rewriting since 1.0, and the OpamFilename types are not particularly convenient, while adding a redundant layer of indirection. So I'd be happy to discuss their replacement if you're so inclined :) Ideally, but this would be a much bigger change, we should have one module with basic definitions (e.g. file names) and system operations, then OpamExternalTools, then one based exclusively on our file name types, and defining the operations like cp and mv just once.

AltGr avatar Feb 12 '18 13:02 AltGr

Also in the medium term, it would be good to start vendoring in things like ocaml-tar or ocaml-git to reduce the dependency on external tools by default. That might affect the consideration of names for this module...

avsm avatar Feb 12 '18 16:02 avsm