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

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

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kit-ty-kate
Copy link
Member

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.

@@ -0,0 +1,125 @@
(**************************************************************************)
(* *)
(* Copyright 2018 OCamlPro *)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't copyright ocamlpro :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just copying the header from the other files. Should I put my name instead ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, in this instance I think it depends - is the module predominantly just refactoring?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I think so.


let to_string (cmd, args) =
(* TODO: Add backslashs to quotes for each args *)
let quote x = if String.contains x ' ' then "'" ^ x ^ "'" else x in
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This quoting logic seems like it might be replicated elsewhere in opam. Bos also has this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll look around. I tried to be as close as the original code as possible (modulo quoting that is added because that seemed broken anyway)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that backslash-quoting in single quotes doesn't work for the shell. You have to write e.g. 'foo'\''bar ; and tabs should also be supported.

@AltGr
Copy link
Member

AltGr commented Feb 12, 2018

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.

@avsm
Copy link
Member

avsm commented Feb 12, 2018

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...

@rjbou rjbou added the PR: WIP Not for merge at this stage label Jun 28, 2019
@AltGr AltGr added this to the Next milestone Mar 11, 2020
@rjbou rjbou added the PR: OUTDATED This PR might still have meaningful content, but is getting stale and might be closed label Mar 11, 2020
@rjbou rjbou removed this from the Next milestone May 20, 2020
@kit-ty-kate kit-ty-kate marked this pull request as draft July 10, 2024 14:45
@kit-ty-kate kit-ty-kate added this to the 2.4.0~alpha1 milestone Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: OUTDATED This PR might still have meaningful content, but is getting stale and might be closed PR: WIP Not for merge at this stage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants