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

Unexpected behaviour when prepending/appending to initially empty variables multiple times #5926

Closed
gridbugs opened this issue Apr 18, 2024 · 1 comment · Fixed by #5935
Closed
Assignees
Milestone

Comments

@gridbugs
Copy link
Contributor

When a variable is initially unset or empty, appending to it and then prepending to it using the colon operators (:= and =:) causes an unexpected leading colon in the result. Prepending to an unset variable followed by appending to it produces the same unexpected result.

Here is a package reproducing the problem:

opam-version: "2.0"

build-env: [

 [ X := "a" ]
# X should be "a:"
 [ X =: "b" ]
# X shoulde be "a:b"
# (assuming opam doesn't add a separator when appending to a variable which ends with a separator)
# X unexpectedly has the value ":a:b"

 [ Y =: "a" ]
# Y should be ":a"
 [ Y := "b" ]
# Y should be "b:a"
# (assuming opam doesn't add a separator when prepending to a variable which starts with a separator)
# Y unexpectedly has the value ":b:a"

]

install: [
    [ "bash" "-c" "echo X=$X >> %{lib}%/foo" ]
    [ "bash" "-c" "echo Y=$Y >> %{lib}%/foo" ]
]

Reproduce by running opam install ./foo.opam && cat _opam/lib/foo which will produce the output:

X=:a:b
Y=:b:a

But according to the documentation the output should be:

X=a:b
Y=b:a

I'm on linux. Reproduced on both 2.1.5 and opam-2.2.0-beta2.

@dra27 dra27 self-assigned this Apr 24, 2024
@dra27 dra27 added this to the 2.2.0~beta3 milestone Apr 25, 2024
dra27 added a commit to dra27/opam that referenced this issue Apr 25, 2024
NV_VARS10 should be a:b and NV_VARS11 should be b:a (no leading colon)
dra27 added a commit to dra27/opam that referenced this issue Apr 29, 2024
NV_VARS10 should be a:b and NV_VARS11 should be b:a (no leading colon)
@dra27
Copy link
Member

dra27 commented Apr 30, 2024

As in #5935, cross-referencing:

The documentation for =: and := I think is not correct since #3404. I think the semantics of := and =: should be as given for empty/unset values but also that they both preserve trailing and leading : in the current value.

Given those semantics:

  • [ X := "a" ] with X empty gives a: (current behaviour is correct)
  • [ X =: "b" ] with X set to a: gives a:b:, adding b at the end, but preserving the trailing colon (current behaviour is incorrect as X=:a:b)
  • [ Y =: "a" ] with Y empty gives :a (current behaviour is correct)
  • [ Y := "b" ] with Y set to :a gives :b:a, adding b at the start, but preserving the leading colon (current behaviour is correct)

So I think that the treatment of X here is incorrect, but the treatment of Y is correct (but the documentation should be updated to reflect this).

dra27 added a commit to dra27/opam that referenced this issue May 2, 2024
- In `opam exec -- env` / `opam env`:
  - NV_VARS_5926_L_2 should be `b::a` (the _L_1 case is correct)
  - NV_VARS_5926_L_4 should be `:a:b` (the _L_3 case is correct)
  - NV_VARS_5926_L_5 and _L_6 should be `b::a` (neither is correct)
  - NV_VARS_5926_L_8 should be `:a:b` (the _L_7 case is correct)
  - All four NV_VARS_5926_M_ all contain `a1:a2` instead of `a1::a2`
  - NV_VARS_5926_T_2 should be `:b:a` (the _T_1 case is correct)
  - NV_VARS_5926_T_4 should be `a::b` (the _T_3 case is correct)
  - NV_VARS_5926_T_6 should be `:b:a` (the _T_5 case is correct)
  - NV_VARS_5926_T_7 and _T_8 should be `a::b` (neither is correct)
- In `opam exec -- opam env --revert`:
  - NV_VARS_5926_L_{2,4,6,8} revert to `a` instead of `:a`
  - NV_VARS_5926_M_{1,2,3,4} should all revert to `a1::a2` where
    - _M_1 and _M_3 only have a single colon (`a1:a2`)
    - _M_2 and _M_4 have reversed the two components! (`a2:a1`)
  - NV_VARS_5926_T_{2,4,6,8} revert to `a` instead of `a:`
  - Note NV_VARS_5926_S_{1,2,3,4} revert to empty rather than `:`
    (this is correct since both interpretations are equivalent and
    opam can't know which it was)
dra27 added a commit to dra27/opam that referenced this issue May 2, 2024
@dra27 dra27 linked a pull request May 2, 2024 that will close this issue
2 tasks
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
- In `opam exec -- env` / `opam env`:
  - NV_VARS_5926_L_2 should be `b::a` (the _L_1 case is correct)
  - NV_VARS_5926_L_4 should be `:a:b` (the _L_3 case is correct)
  - NV_VARS_5926_L_5 and _L_6 should be `b::a` (neither is correct)
  - NV_VARS_5926_L_8 should be `:a:b` (the _L_7 case is correct)
  - All four NV_VARS_5926_M_ all contain `a1:a2` instead of `a1::a2`
  - NV_VARS_5926_T_2 should be `:b:a` (the _T_1 case is correct)
  - NV_VARS_5926_T_4 should be `a::b` (the _T_3 case is correct)
  - NV_VARS_5926_T_6 should be `:b:a` (the _T_5 case is correct)
  - NV_VARS_5926_T_7 and _T_8 should be `a::b` (neither is correct)
- In `opam exec -- opam env --revert`:
  - NV_VARS_5926_L_{2,4,6,8} revert to `a` instead of `:a`
  - NV_VARS_5926_M_{1,2,3,4} should all revert to `a1::a2` where
    - _M_1 and _M_3 only have a single colon (`a1:a2`)
    - _M_2 and _M_4 have reversed the two components! (`a2:a1`)
  - NV_VARS_5926_T_{2,4,6,8} revert to `a` instead of `a:`
  - Note NV_VARS_5926_S_{1,2,3,4} revert to empty rather than `:`
    (this is correct since both interpretations are equivalent and
    opam can't know which it was)
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
- In `opam exec -- env` / `opam env`:
  - NV_VARS_5926_L_2 should be `b::a` (the _L_1 case is correct)
  - NV_VARS_5926_L_4 should be `:a:b` (the _L_3 case is correct)
  - NV_VARS_5926_L_5 and _L_6 should be `b::a` (neither is correct)
  - NV_VARS_5926_L_8 should be `:a:b` (the _L_7 case is correct)
  - All four NV_VARS_5926_M_ all contain `a1:a2` instead of `a1::a2`
  - NV_VARS_5926_T_2 should be `:b:a` (the _T_1 case is correct)
  - NV_VARS_5926_T_4 should be `a::b` (the _T_3 case is correct)
  - NV_VARS_5926_T_6 should be `:b:a` (the _T_5 case is correct)
  - NV_VARS_5926_T_7 and _T_8 should be `a::b` (neither is correct)
- In `opam exec -- opam env --revert`:
  - NV_VARS_5926_L_{2,4,6,8} revert to `a` instead of `:a`
  - NV_VARS_5926_M_{1,2,3,4} should all revert to `a1::a2` where
    - _M_1 and _M_3 only have a single colon (`a1:a2`)
    - _M_2 and _M_4 have reversed the two components! (`a2:a1`)
  - NV_VARS_5926_T_{2,4,6,8} revert to `a` instead of `a:`
  - Note NV_VARS_5926_S_{1,2,3,4} revert to empty rather than `:`
    (this is correct since both interpretations are equivalent and
    opam can't know which it was)
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
- In `opam exec -- env` / `opam env`:
  - NV_VARS_5926_L_2 should be `b::a` (the _L_1 case is correct)
  - NV_VARS_5926_L_4 should be `:a:b` (the _L_3 case is correct)
  - NV_VARS_5926_L_5 and _L_6 should be `b::a` (neither is correct)
  - NV_VARS_5926_L_8 should be `:a:b` (the _L_7 case is correct)
  - All four NV_VARS_5926_M_ all contain `a1:a2` instead of `a1::a2`
  - NV_VARS_5926_T_2 should be `:b:a` (the _T_1 case is correct)
  - NV_VARS_5926_T_4 should be `a::b` (the _T_3 case is correct)
  - NV_VARS_5926_T_6 should be `:b:a` (the _T_5 case is correct)
  - NV_VARS_5926_T_7 and _T_8 should be `a::b` (neither is correct)
- In `opam exec -- opam env --revert`:
  - NV_VARS_5926_L_{2,4,6,8} revert to `a` instead of `:a`
  - NV_VARS_5926_M_{1,2,3,4} should all revert to `a1::a2` where
    - _M_1 and _M_3 only have a single colon (`a1:a2`)
    - _M_2 and _M_4 have reversed the two components! (`a2:a1`)
  - NV_VARS_5926_T_{2,4,6,8} revert to `a` instead of `a:`
  - Note NV_VARS_5926_S_{1,2,3,4} revert to empty rather than `:`
    (this is correct since both interpretations are equivalent and
    opam can't know which it was)
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
- In `opam exec -- env` / `opam env`:
  - NV_VARS_5926_L_2 should be `b::a` (the _L_1 case is correct)
  - NV_VARS_5926_L_4 should be `:a:b` (the _L_3 case is correct)
  - NV_VARS_5926_L_5 and _L_6 should be `b::a` (neither is correct)
  - NV_VARS_5926_L_8 should be `:a:b` (the _L_7 case is correct)
  - All four NV_VARS_5926_M_ all contain `a1:a2` instead of `a1::a2`
  - NV_VARS_5926_T_2 should be `:b:a` (the _T_1 case is correct)
  - NV_VARS_5926_T_4 should be `a::b` (the _T_3 case is correct)
  - NV_VARS_5926_T_6 should be `:b:a` (the _T_5 case is correct)
  - NV_VARS_5926_T_7 and _T_8 should be `a::b` (neither is correct)
- In `opam exec -- opam env --revert`:
  - NV_VARS_5926_L_{2,4,6,8} revert to `a` instead of `:a`
  - NV_VARS_5926_M_{1,2,3,4} should all revert to `a1::a2` where
    - _M_1 and _M_3 only have a single colon (`a1:a2`)
    - _M_2 and _M_4 have reversed the two components! (`a2:a1`)
  - NV_VARS_5926_T_{2,4,6,8} revert to `a` instead of `a:`
  - Note NV_VARS_5926_S_{1,2,3,4} revert to empty rather than `:`
    (this is correct since both interpretations are equivalent and
    opam can't know which it was)
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
- In `opam exec -- env` / `opam env`:
  - NV_VARS_5926_L_2 should be `b::a` (the _L_1 case is correct)
  - NV_VARS_5926_L_4 should be `:a:b` (the _L_3 case is correct)
  - NV_VARS_5926_L_5 and _L_6 should be `b::a` (neither is correct)
  - NV_VARS_5926_L_8 should be `:a:b` (the _L_7 case is correct)
  - All four NV_VARS_5926_M_ all contain `a1:a2` instead of `a1::a2`
  - NV_VARS_5926_T_2 should be `:b:a` (the _T_1 case is correct)
  - NV_VARS_5926_T_4 should be `a::b` (the _T_3 case is correct)
  - NV_VARS_5926_T_6 should be `:b:a` (the _T_5 case is correct)
  - NV_VARS_5926_T_7 and _T_8 should be `a::b` (neither is correct)
- In `opam exec -- opam env --revert`:
  - NV_VARS_5926_L_{2,4,6,8} revert to `a` instead of `:a`
  - NV_VARS_5926_M_{1,2,3,4} should all revert to `a1::a2` where
    - _M_1 and _M_3 only have a single colon (`a1:a2`)
    - _M_2 and _M_4 have reversed the two components! (`a2:a1`)
  - NV_VARS_5926_T_{2,4,6,8} revert to `a` instead of `a:`
  - Note NV_VARS_5926_S_{1,2,3,4} revert to empty rather than `:`
    (this is correct since both interpretations are equivalent and
    opam can't know which it was)
rjbou pushed a commit to rjbou/opam that referenced this issue May 10, 2024
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
- In `opam exec -- env` / `opam env`:
  - NV_VARS_5926_L_2 should be `b::a` (the _L_1 case is correct)
  - NV_VARS_5926_L_4 should be `:a:b` (the _L_3 case is correct)
  - NV_VARS_5926_L_5 and _L_6 should be `b::a` (neither is correct)
  - NV_VARS_5926_L_8 should be `:a:b` (the _L_7 case is correct)
  - All four NV_VARS_5926_M_ all contain `a1:a2` instead of `a1::a2`
  - NV_VARS_5926_T_2 should be `:b:a` (the _T_1 case is correct)
  - NV_VARS_5926_T_4 should be `a::b` (the _T_3 case is correct)
  - NV_VARS_5926_T_6 should be `:b:a` (the _T_5 case is correct)
  - NV_VARS_5926_T_7 and _T_8 should be `a::b` (neither is correct)
- In `opam exec -- opam env --revert`:
  - NV_VARS_5926_L_{2,4,6,8} revert to `a` instead of `:a`
  - NV_VARS_5926_M_{1,2,3,4} should all revert to `a1::a2` where
    - _M_1 and _M_3 only have a single colon (`a1:a2`)
    - _M_2 and _M_4 have reversed the two components! (`a2:a1`)
  - NV_VARS_5926_T_{2,4,6,8} revert to `a` instead of `a:`
  - Note NV_VARS_5926_S_{1,2,3,4} revert to empty rather than `:`
    (this is correct since both interpretations are equivalent and
    opam can't know which it was)
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
:= and =: now ensure that an empty directory empty is always introduced
then all the append operators ensure that an empty directory entry is
maintained (see env.test changes).
dra27 added a commit to dra27/opam that referenced this issue May 14, 2024
:= and =: now ensure that an empty directory empty is always introduced
then all the append operators ensure that an empty directory entry is
maintained (see env.test changes).
dra27 added a commit to dra27/opam that referenced this issue May 15, 2024
- In `opam exec -- env` / `opam env`:
  - NV_VARS_5926_L_2 should be `b::a` (the _L_1 case is correct)
  - NV_VARS_5926_L_4 should be `:a:b` (the _L_3 case is correct)
  - NV_VARS_5926_L_5 and _L_6 should be `b::a` (neither is correct)
  - NV_VARS_5926_L_8 should be `:a:b` (the _L_7 case is correct)
  - All four NV_VARS_5926_M_ all contain `a1:a2` instead of `a1::a2`
  - NV_VARS_5926_T_2 should be `:b:a` (the _T_1 case is correct)
  - NV_VARS_5926_T_4 should be `a::b` (the _T_3 case is correct)
  - NV_VARS_5926_T_6 should be `:b:a` (the _T_5 case is correct)
  - NV_VARS_5926_T_7 and _T_8 should be `a::b` (neither is correct)
- In `opam exec -- opam env --revert`:
  - NV_VARS_5926_L_{2,4,6,8} revert to `a` instead of `:a`
  - NV_VARS_5926_M_{1,2,3,4} should all revert to `a1::a2` where
    - _M_1 and _M_3 only have a single colon (`a1:a2`)
    - _M_2 and _M_4 have reversed the two components! (`a2:a1`)
  - NV_VARS_5926_T_{2,4,6,8} revert to `a` instead of `a:`
  - Note NV_VARS_5926_S_{1,2,3,4} revert to empty rather than `:`
    (this is correct since both interpretations are equivalent and
    opam can't know which it was)
dra27 added a commit to dra27/opam that referenced this issue May 15, 2024
:= and =: now ensure that an empty directory empty is always introduced
then all the append operators ensure that an empty directory entry is
maintained (see env.test changes).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants