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.

Sign up for GitHub

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

init scripts: revert environment before settings new one, on session loading #5036

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

init scripts: revert environment before settings new one, on session loading #5036

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b729c9
reftests: add scripts test, simple display
rjbou Feb 2, 2022
f16439a
init scripts: on `init.sh', add a check if environment is already set…
rjbou Feb 2, 2022
814aa92
reftests: update scripts
rjbou Feb 2, 2022
435b381
updates changes
rjbou Feb 2, 2022
b326213
fixup! reftests: add scripts test, simple display
rjbou Feb 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions master_changes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ users)

## Shell
* fish: fix deprecated redirection syntax `^` [#4736 @vzaliva]
* Before sourcing `variables.sh`, check if opam env is set (`OPAM_SWITCH_PREFIX`) and revert it in that case [#5034 @rjbou - fix #4649]

## Doc
* Standardise `macOS` use [#4782 @kit-ty-kate]
Expand Down
102 changes: 79 additions & 23 deletions src/state/opamEnv.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,45 +498,101 @@ let env_hook_script shell =
^ script)
(env_hook_script_base shell)

let source root shell f =
let if_expr ?(level=0) ~icond ~ithen ?ielse shell =
let pad = String.concat "" (List.init (level*2) (fun _ -> " ")) in
let ielse_expr else_opt = match else_opt with
| None -> ""
| Some e -> Printf.sprintf "else\n %s%s\n" pad e
in
let ithen = pad ^ ithen ^ "\n" in
match shell with
| SH_sh | SH_bash ->
Printf.sprintf "%sif [ %s ]; then\n %s%s%sfi"
pad icond ithen (ielse_expr ielse) pad
| SH_zsh ->
Printf.sprintf "%sif [[ %s ]]; then\n %s%s%sfi"
pad icond ithen (ielse_expr ielse) pad
Copy link
Member

Choose a reason for hiding this comment

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

Why is the bash / zsh distinction needed here ? I thought both syntaxes existed in both, with [[ allowing more specific built-ins. [ should behave the same for zsh as for bash.

Copy link
Collaborator Author

@rjbou rjbou Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Simply because I took if expression from if_interactive_script, and it set this sh/zsh difference

| SH_csh ->
Printf.sprintf "%sif ( %s ) then\n %s%s%sendif"
pad icond ithen (ielse_expr ielse) pad
| SH_fish ->
Printf.sprintf "%sif %s\n %s%s%send"
pad icond ithen (ielse_expr ielse) pad

let source root shell ?pre f =
let fname = OpamFilename.to_string (OpamPath.init root // f) in
match shell with
| SH_csh ->
Printf.sprintf "if ( -f %s ) source %s >& /dev/null\n" fname fname
let source = Printf.sprintf "source %s >& /dev/null" fname in
(match pre with
| None -> Printf.sprintf "if ( -f %s ) %s" fname source
| Some pre ->
if_expr SH_csh ~icond:(Printf.sprintf "-f %s" fname)
~ithen:(Printf.sprintf "%s\n %s" pre source))
| SH_fish ->
Printf.sprintf "source %s > /dev/null 2> /dev/null; or true\n" fname
let source = Printf.sprintf "source %s > /dev/null 2> /dev/null" fname in
(match pre with
| None -> Printf.sprintf "%s; or true" source
| Some pre ->
Printf.sprintf "begin\n%s\n %s\nend; or true" pre source)
| SH_sh | SH_bash ->
Printf.sprintf "test -r %s && . %s > /dev/null 2> /dev/null || true\n"
fname fname
let source = Printf.sprintf ". %s > /dev/null 2> /dev/null" fname in
(match pre with
| None -> Printf.sprintf "test -r %s && %s || true" fname source
| Some pre ->
Printf.sprintf "test -r %s && { %s;\n %s; } || true" fname pre source)
| SH_zsh ->
Printf.sprintf "[[ ! -r %s ]] || source %s > /dev/null 2> /dev/null\n"
fname fname
let source = Printf.sprintf "source %s > /dev/null 2> /dev/null" fname in
(match pre with
| None -> Printf.sprintf "[[ ! -r %s ]] || %s" fname source
| Some pre ->
Printf.sprintf "[[ ! -r %s ]] || { %s;\n %s; }"
fname pre source)

let source_variables root shell =
let variable_file = (variables_file shell) in
let iif ?(level=1) icond ithen = if_expr ~level shell ~icond ~ithen in
let revert =
match shell with
| SH_sh | SH_bash ->
iif {|"x$OPAM_SWITCH_PREFIX" != "x"|}
"eval $(opam env --revert --shell=bash --readonly 2> /dev/null <&- )"
| SH_zsh ->
iif {|-n "$OPAM_SWITCH_PREFIX"|}
rjbou marked this conversation as resolved.
Show resolved Hide resolved
"eval $(opam env --revert --shell=zsh --readonly 2> /dev/null <&- )"
| SH_csh ->
Printf.sprintf "%s\n%s"
(* XXX duplicated with variables.sh *)
(iif "! ${?OPAM_SWITCH_PREFIX}" {|setenv OPAM_SWITCH_PREFIX ""|})
(iif {|"x$OPAM_SWITCH_PREFIX" != "x"|}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Same than bash safeguard, needed?

"eval `opam env --revert --shell=csh --readonly`")
| SH_fish ->
iif {|test -n "$OPAM_SWITCH_PREFIX"|}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

nox safeguard trick, is it needed?

"eval (opam env --revert --shell=fish --readonly 2> /dev/null)"
in
source root shell ~pre:revert variable_file


let if_interactive_script shell t e =
let ielse else_opt = match else_opt with
| None -> ""
| Some e -> Printf.sprintf "else\n %s" e
let if_interactive_script ~ithen ?ielse shell =
let icond =
match shell with
| SH_sh| SH_bash -> "-t 0"
| SH_zsh -> "-o interactive"
| SH_csh -> "$?prompt"
| SH_fish -> "isatty"
in
match shell with
| SH_sh| SH_bash ->
Printf.sprintf "if [ -t 0 ]; then\n %s%sfi\n" t @@ ielse e
| SH_zsh ->
Printf.sprintf "if [[ -o interactive ]]; then\n %s%sfi\n" t @@ ielse e
| SH_csh ->
Printf.sprintf "if ( $?prompt ) then\n %s%sendif\n" t @@ ielse e
| SH_fish ->
Printf.sprintf "if isatty\n %s%send\n" t @@ ielse e
if_expr shell ~icond ~ithen ?ielse

let init_script root shell =
let interactive =
List.map (source root shell) @@
OpamStd.List.filter_some [complete_file shell; env_hook_file shell]
in
String.concat "\n" @@
String.concat "\n\n" @@
(if interactive <> [] then
[if_interactive_script shell (String.concat "\n " interactive) None]
[if_interactive_script shell ~ithen:(String.concat "\n " interactive)]
else []) @
[source root shell (variables_file shell)]
[source_variables root shell]

let string_of_update st shell updates =
let fenv = OpamPackageVar.resolve st in
Expand Down
17 changes: 17 additions & 0 deletions tests/reftests/dune.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,23 @@
%{targets}
(run ./run.exe %{bin:opam} %{dep:remove.test} %{read-lines:testing-env}))))

(rule
(alias reftest-scripts)
(action
(diff scripts.test scripts.out)))

(alias
(name reftest)
(deps (alias reftest-scripts)))

(rule
(targets scripts.out)
(deps root-N0REP0)
(action
(with-stdout-to
%{targets}
(run ./run.exe %{bin:opam} %{dep:scripts.test} %{read-lines:testing-env}))))

(rule
(alias reftest-show)
(action
Expand Down
Loading