Skip to content

Commit

Permalink
Merge pull request ocaml#4904 from rjbou/var-shadow
Browse files Browse the repository at this point in the history 
Warn when setting a variable that shadow an option
  • Loading branch information
@rjbou
rjbou authored Sep 4, 2024
2 parents 86f2dbd + 9bf7193 commit 26f7462
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 20 deletions.
1 change: 1 addition & 0 deletions master_changes.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ users)

## Var/Option
* Fix the value of the 'arch' variable when the current OS is 32bit on a 64bit machine [#5950 @kit-ty-kate - fix #5949]
* Warn when setting a variable if an option is shadowed [#4904 @rjbou - fix #4730]

## Update / Upgrade

Expand Down
13 changes: 13 additions & 0 deletions src/client/opamConfigCommand.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -936,6 +936,19 @@ type ('var,'config) var_confset =
}

let set_var var value conf =
let svar = OpamVariable.Full.to_string var in
let global_option, switch_option =
let exists f = List.exists (fun (name, _) -> String.equal svar name) f in
(exists OpamFile.Config.fields),
(exists OpamFile.Switch_config.fields)
in
if global_option || switch_option then
(let scope = if global_option then "global" else "switch" in
OpamConsole.warning
"You are setting a variable that has the same name as a %s option.\n\
Did you mean to use `opam option` instead? You can revert this variable \
using: 'opam var %s= --%s'\n"
scope svar scope);
let conf = conf (OpamVariable.Full.variable var) in
let global_vars = conf.stv_vars in
let rest = List.filter (fun v -> not (conf.stv_find v)) global_vars in
Expand Down
92 changes: 72 additions & 20 deletions tests/reftests/var-option.test
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,8 @@ Removed variable group in switch var-option
### opam option sys-pkg-manager-cmd= --global | grep -v configuration
### opam var arch=x86_64 --global
Added '[arch "x86_64" "Set through 'opam var'"]' to field global-variables in global configuration
### opam var jobs=7 --global
Added '[jobs "7" "Set through 'opam var'"]' to field global-variables in global configuration
### opam var make=make --global
Added '[make "make" "Set through 'opam var'"]' to field global-variables in global configuration
### opam var opam-version=68.79 --global
Added '[opam-version "68.79" "Set through 'opam var'"]' to field global-variables in global configuration
### opam var os=linux --global
Added '[os "linux" "Set through 'opam var'"]' to field global-variables in global configuration
### opam var os-distribution=lorem --global
Expand All @@ -45,6 +41,8 @@ Set to '[]' the field wrap-build-commands in global configuration
Set to '[]' the field wrap-install-commands in global configuration
### opam option wrap-remove-commands=[] --global
Set to '[]' the field wrap-remove-commands in global configuration
### opam option jobs=7 --global
Set to '7' the field jobs in global configuration
### opam install i-got-the-variables --yes
The following actions will be performed:
=== install 2 packages
Expand All @@ -55,14 +53,13 @@ The following actions will be performed:
-> installed base.2
-> installed i-got-the-variables.2.4.6
Done.
### opam var | " *" -> " " | "[.]exe " -> ""
### opam var | " *" -> " " | "[.]exe " -> "" | "opam-version.*" -> '\c'

<><> Global opam variables ><><><><><><><><><><><><><><><><><><><><><><><><><><>
arch x86_64 # Set through 'opam var'
exe # Suffix needed for executable filenames (Windows)
jobs 7 # Set through 'opam var'
jobs 7 # The number of parallel jobs set up in opam configuration
make make # Set through 'opam var'
opam-version 68.79 # Set through 'opam var'
os linux # Set through 'opam var'
os-distribution lorem # Set through 'opam var'
os-family ipsum # Set through 'opam var'
Expand Down Expand Up @@ -113,12 +110,11 @@ etc ${BASEDIR}/OPAM/var-option/etc
man ${BASEDIR}/OPAM/var-option/man
toplevel ${BASEDIR}/OPAM/var-option/lib/toplevel
stublibs ${BASEDIR}/OPAM/var-option/lib/stublibs
### opam var --global | " *" -> " " | "[.]exe " -> ""
### opam var --global | " *" -> " " | "[.]exe " -> "" | "opam-version.*" -> '\c'
arch x86_64 # Set through 'opam var'
exe # Suffix needed for executable filenames (Windows)
jobs 7 # Set through 'opam var'
jobs 7 # The number of parallel jobs set up in opam configuration
make make # Set through 'opam var'
opam-version 68.79 # Set through 'opam var'
os linux # Set through 'opam var'
os-distribution lorem # Set through 'opam var'
os-family ipsum # Set through 'opam var'
Expand Down Expand Up @@ -231,7 +227,7 @@ depext-run-installs true
download-command {}
download-jobs 3
git-location {}
jobs {}
jobs 7
post-build-commands {}
post-install-commands {}
post-remove-commands {}
Expand Down Expand Up @@ -275,7 +271,9 @@ wrap-remove-commands {}
[ERROR] Field or section download-jobs not found
# Return code 5 #
### opam option jobs
7
### opam option jobs --global
7
### opam option jobs --switch var-option
[ERROR] Field or section jobs not found
# Return code 5 #
Expand Down Expand Up @@ -488,7 +486,7 @@ Reverted field archive-mirrors in global configuration
### opam option archive-mirrors
### # Check that eval-variable is removed when a global variable is removed
### opam option global-variables
[[foo "global-foo" "Set through 'opam var'"] [bin "global-bin" "Set through 'opam var'"] [os-version "dolor" "Set through 'opam var'"] [os-family "ipsum" "Set through 'opam var'"] [os-distribution "lorem" "Set through 'opam var'"] [os "linux" "Set through 'opam var'"] [opam-version "68.79" "Set through 'opam var'"] [make "make" "Set through 'opam var'"] [jobs "7" "Set through 'opam var'"] [arch "x86_64" "Set through 'opam var'"] [sys-ocaml-version "4.08.0" "Set through 'opam var'"]]
[[foo "global-foo" "Set through 'opam var'"] [bin "global-bin" "Set through 'opam var'"] [os-version "dolor" "Set through 'opam var'"] [os-family "ipsum" "Set through 'opam var'"] [os-distribution "lorem" "Set through 'opam var'"] [os "linux" "Set through 'opam var'"] [make "make" "Set through 'opam var'"] [arch "x86_64" "Set through 'opam var'"] [sys-ocaml-version "4.08.0" "Set through 'opam var'"]]
### opam option 'eval-variables=[dolore ["mania"] "alica"]'
Set to '[dolore ["mania"] "alica"]' the field eval-variables in global configuration
### opam option eval-variables
Expand All @@ -500,8 +498,8 @@ Removed variable dolore in global configuration
### opam option eval-variables
[]
### opam option global-variables
[[foo "global-foo" "Set through 'opam var'"] [bin "global-bin" "Set through 'opam var'"] [os-version "dolor" "Set through 'opam var'"] [os-family "ipsum" "Set through 'opam var'"] [os-distribution "lorem" "Set through 'opam var'"] [os "linux" "Set through 'opam var'"] [opam-version "68.79" "Set through 'opam var'"] [make "make" "Set through 'opam var'"] [jobs "7" "Set through 'opam var'"] [arch "x86_64" "Set through 'opam var'"] [sys-ocaml-version "4.08.0" "Set through 'opam var'"]]
### # Check uniable operations
[[foo "global-foo" "Set through 'opam var'"] [bin "global-bin" "Set through 'opam var'"] [os-version "dolor" "Set through 'opam var'"] [os-family "ipsum" "Set through 'opam var'"] [os-distribution "lorem" "Set through 'opam var'"] [os "linux" "Set through 'opam var'"] [make "make" "Set through 'opam var'"] [arch "x86_64" "Set through 'opam var'"] [sys-ocaml-version "4.08.0" "Set through 'opam var'"]]
### # Check disallowed operations
### opam option 'variables+={esse: "cillum"}'
[ERROR] Field variables can't be directly appended to, use `opam var` instead
# Return code 2 #
Expand Down Expand Up @@ -534,10 +532,66 @@ jobs download-command download-jobs archive-mirrors solver-criteria solver-upgra
[ERROR] There is no option named 'bar'. The allowed options are:
synopsis setenv depext-bypass pre-build-commands pre-install-commands pre-remove-commands pre-session-commands wrap-build-commands wrap-install-commands wrap-remove-commands post-build-commands post-install-commands post-remove-commands post-session-commands variables
# Return code 2 #
### # Check option shadowing
### opam option download-jobs
1
### opam var download-jobs
[ERROR] Variable download-jobs not found in the global configuration
# Return code 5 #
### opam var download-jobs=42 --global
[WARNING] You are setting a variable that has the same name as a global option.
Did you mean to use `opam option` instead? You can revert this variable using: 'opam var download-jobs= --global'

Added '[download-jobs "42" "Set through 'opam var'"]' to field global-variables in global configuration
### opam option download-jobs --global
1
### opam var download-jobs --global
42
### opam var download-jobs=44 --switch var-option
[WARNING] You are setting a variable that has the same name as a global option.
Did you mean to use `opam option` instead? You can revert this variable using: 'opam var download-jobs= --global'

Added 'download-jobs: "44"' to field variables in switch var-option
### opam var download-jobs --switch var-option
44
### opam var synopsis --global
[ERROR] Variable synopsis not found in the global configuration
# Return code 5 #
### opam var 'synopsis="lorem ipsum"' --global
[WARNING] You are setting a variable that has the same name as a switch option.
Did you mean to use `opam option` instead? You can revert this variable using: 'opam var synopsis= --switch'

Added '[synopsis "\"lorem ipsum\"" "Set through 'opam var'"]' to field global-variables in global configuration
### opam var synopsis --global
"lorem ipsum"
### opam option synopsis --switch var-option
""
### opam var synopsis --switch var-option
[ERROR] Variable synopsis not found in switch var-option
# Return code 5 #
### opam var 'synopsis="ipsum"' --switch var-option
[WARNING] You are setting a variable that has the same name as a switch option.
Did you mean to use `opam option` instead? You can revert this variable using: 'opam var synopsis= --switch'

Added 'synopsis: "\"ipsum\""' to field variables in switch var-option
### opam option synopsis --switch var-option
""
### opam var synopsis --switch var-option
"ipsum"
### opam var synopsis= --global -y
[WARNING] You are setting a variable that has the same name as a switch option.
Did you mean to use `opam option` instead? You can revert this variable using: 'opam var synopsis= --switch'

Removed variable synopsis in global configuration
### opam var download-jobs= --global -y
[WARNING] You are setting a variable that has the same name as a global option.
Did you mean to use `opam option` instead? You can revert this variable using: 'opam var download-jobs= --global'

Removed variable download-jobs in global configuration
### : when no switch is present
### opam switch remove var-option -y
Switch var-option and all its packages will be wiped. Are you sure? [y/n] y
### opam var | ' +#.*' -> '' | " +[.]exe" -> ""
### opam var | ' +#.*' -> '' | " +[.]exe" -> "" | "opam-version.*" -> '\c'

<><> Global opam variables ><><><><><><><><><><><><><><><><><><><><><><><><><><>
arch x86_64
Expand All @@ -546,7 +600,6 @@ exe
foo global-foo
jobs 7
make make
opam-version 68.79
os linux
os-distribution lorem
os-family ipsum
Expand Down Expand Up @@ -577,14 +630,13 @@ PKG:hash
PKG:dev
PKG:build-id
PKG:opamfile
### opam var --global | ' +#.*' -> '' | " +[.]exe" -> ""
### opam var --global | ' +#.*' -> '' | " +[.]exe" -> "" | "opam-version.*" -> '\c'
arch x86_64
bin global-bin
exe
foo global-foo
jobs 7
make make
opam-version 68.79
os linux
os-distribution lorem
os-family ipsum
Expand Down Expand Up @@ -615,7 +667,7 @@ depext-run-installs true
download-command {}
download-jobs 1
git-location {}
jobs {}
jobs 7
post-build-commands {}
post-install-commands {}
post-remove-commands {}
Expand Down Expand Up @@ -647,7 +699,7 @@ depext-run-installs true
download-command {}
download-jobs 1
git-location {}
jobs {}
jobs 7
post-build-commands {}
post-install-commands {}
post-remove-commands {}
Expand Down

0 comments on commit 26f7462

Please sign in to comment.