Fix reverting additions to PATH-like variables #4861

dra27 · 2021-10-07T19:07:41Z

For environment variables which can be split (e.g. PATH on :), OpamEnv.unzip_to attempts to find the point in the variable at which a value was added. This is fine if a single value was added, but fails if the addition was multiple values (for example, if a setenv instruction added two directories to PATH in one +=).

This is fixed by first splitting the value being searched according to the same rule as the environmen variable and ensuring they all match in order.

Finalise test case
Double-check that it definitely fixes the Windows issue (I've rebased and edited it on Linux)

dra27 · 2021-10-07T19:13:25Z

This opam file provides the basis for a test:

opam-version: "2.0"
license: "ISC"
synopsis: "Test package for setenv field"
description: "Test case for setenv field"
maintainer: "platform@lists.ocaml.org"
setenv: [
  [CAML_LD_LIBRARY_PATH += "%{_:doc}%:%{_:share}%"]
]
homepage: "https://opam.ocaml.org"
bug-reports: "https://github.com/ocaml/opam/issues"
authors: ["David Allsopp"]

This package needs to be installed on its own (or the ocaml package masks the problem) with opam switch create --repos=setenv-test-repo setenv-test --packages=setenv. With current opam opam env --revert will not include CAML_LD_LIBRARY_PATH (because opam fails to detect the need to revert the environment update at all) and with this PR it will include export CAML_LD_LIBRARY_PATH=''.

@rjbou - I wasn't sure where best to integrate this as a reftest... perhaps we can share some of the infrastructure used for the root-versions test in order to set-up a repository with just this package, or is there already an easier way in another test?

dra27 · 2021-10-08T08:00:05Z

The AppVeyor failure is serious, but not to do with this PR; the tests failures are to do with the gforge shutdown at Inria

For environment variables which can be split (e.g. `PATH` on `:`), OpamEnv.unzip_to attempts to find the point in the variable at which a value was added. This is fine if a _single_ value was added, but fails if the addition was multiple values (for example, if a setenv instruction added two directories to `PATH` in one `+=`). This is fixed by first splitting the value being searched according to the same rule as the environmen variable and ensuring they all match in order.

As in this commit, there should be no output captured from the opam env --revert invocation.

When reverting [FOO += "value"], it is necessary to determine what to do if "value" is of the form "<value1><sep><value2>". Because FOO itself will have been split at <sep>, we will never match "<value1><sep><value2>" so it is necessary to split value at <sep> as well (this is the crux of ocaml#4861). However, the interaction of this and x-env-path-rewrite is incorrect. If x-env-path-rewrite has [FOO false] (the `norewrite case), then the value must be split according to the default interpretation of FOO (which is how it was added). If x-env-path-rewrite has a rewrite rule for FOO, then opam has already assumed that value refers to a single directory (the host/target and quoting transformation options all being this way). If x-env-path-rewrite does not have any rule (and does have have [FOO false]) then we are in essence in the backwards-compatible case, and must split value. In order to stop poor interaction between this and opam itself injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.) are explicitly expanded so that in unzip_to they are treated as though they had an x-env-path-rewrite rule given (and are therefore not split).

As in this commit, there should be no output captured from the opam env --revert invocation.

The cases of this match were the wrong way around. Note that this regresses ocaml#4861's quoted directory test.

When reverting [FOO += "value"], it is necessary to determine what to do if "value" is of the form "<value1><sep><value2>". Because FOO itself will have been split at <sep>, we will never match "<value1><sep><value2>" so it is necessary to split value at <sep> as well (this is the crux of ocaml#4861). However, the interaction of this and x-env-path-rewrite is incorrect. If x-env-path-rewrite has [FOO false] (the `norewrite case), then the value must be split according to the default interpretation of FOO (which is how it was added). If x-env-path-rewrite has a rewrite rule for FOO, then opam has already assumed that value refers to a single directory (the host/target and quoting transformation options all being this way). If x-env-path-rewrite does not have any rule (and does have have [FOO false]) then we are in essence in the backwards-compatible case, and must split value. In order to stop poor interaction between this and opam itself injecting Windows paths, opam's updates in OpamEnv (to MANPATH, etc.) are explicitly expanded so that in unzip_to they are treated as though they had an x-env-path-rewrite rule given (and are therefore not split). Combined with the previous fixes, this addresses ocaml#5838 without regressing ocaml#4861.