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

Fix empty updates in dynamic init scripts #6198

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dra27
Copy link
Member

@dra27 dra27 commented Sep 16, 2024

An update of the form:

FOO += ""

is not supposed to do anything to the environment (#5350). However, these updates were being written to the init scripts in opam-init (variables.sh, etc.). This PR fixes that, and consequently the problem with mingw-w64-shims reported in dra27/mingw-w64-shims#2 and #6053 (comment).

Before this PR, for a switch with mingw-w64-shims and only the x86_64 installation, variables.sh has:

# Updated by package mingw-w64-shims
PATH='C:\Devel\Roots\test-empty\.cygwin\root\usr\x86_64-w64-mingw32\sys-root\mingw\bin':"$PATH"; export PATH;
# Updated by package mingw-w64-shims
PATH='':"$PATH"; export PATH;

which is wrong, but after just:

# Updated by package mingw-w64-shims
PATH='C:\Devel\Roots\test-empty\.cygwin\root\usr\x86_64-w64-mingw32\sys-root\mingw\bin':"$PATH"; export PATH;

Both before and after, the switch's environment file correctly has both entries:

PATH    +=      C:\\Devel\\Roots\\test-empty\\.cygwin\\root\\usr\\x86_64-w64-mingw32\\sys-root\\mingw\\bin      Updated\ by\ package\ mingw-w64-shims
PATH    +=      @       Updated\ by\ package\ mingw-w64-shims

@dra27 dra27 added this to the 2.3.0~alpha2 milestone Sep 23, 2024
opam already ensured that empty segments are never added to environment
variables with opam env, but the filter was not there for writing the
dynamic init scripts.
Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

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

modulo the fixup commit i've just pushed (feel free to remove it or squash it if you disagree or agree with it), that looks ok to me, although i'm not an expert in this part of the code.

Could you add a test in the reftests to avoid future regressions? From afar it looks reasonably easy to craft a test for this particular case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants