search

ocaml / opam

opam is a source-based package manager. It supports multiple simultaneous compiler installations, flexible package constraints, and a Git-friendly development workflow.
https://opam.ocaml.org
Other
1.21k stars 348 forks source link

Relax warning 41 for package variables guarded by a :installed filter #5927

Closed dra27 closed 1 month ago

dra27 commented 2 months ago

The compiler packages contain the following snippet:

depends: [
  "flexdll" {>= "0.42" & os = "win32"}
]
build: [
  [
    "./configure"
    "--with-flexdll=%{flexdll:share}%" {flexdll:installed}
  ]
]

but the use of flexdll:share triggers lint warning 41 because the flexdll dependency is guarded by os. However, this warning is a bit obtuse given the argument is guarded by flexdll:installed (and :installed explicitly doesn't trigger lint warning 41).

This PR alters lint warning 41 so that when it evaluates the list of variables used in commands, it scans the filters for instances of package:installed and then performs a partial evaluation of the filter with that package:installed variable explicitly set to false (but nothing else set in the environment). If the filter reduces to false, then all instances of package:var guarded by the filter are ignored.

The first commit adds a test case showing the present behaviour, then the fix shows two of the packages warned being eliminated.

dra27 commented 2 months ago

@kit-ty-kate rightly points out a timing issue which concretely invalidates one of the test cases. The filter above should repeat the guard of the dependency, i.e. instead of:

    "--with-flexdll=%{flexdll:share}%" {flexdll:installed}

it should be:

    "--with-flexdll=%{flexdll:share}%" {flexdll:installed & os = "win32"}

this also concretely rules out this line from the test:

  ["%{foo:share}%%{bar:share}%" {bar:installed}] {foo:installed}

since bar is not referred to in the dependency formula in any way.

I'm wondering if this is in fact more related to #5928 - in order to use the variables, we need both that it's unequivocally guarded by :installed (as here) and that the atom appears in some way in either depends or depopt (to enforce the ordering).

Regardless, the test case of bar is a deficiency of this PR, so I'm putting it back to draft status for now.

dra27 commented 1 month ago

Additional commit added which tweaks the check slightly. Now: