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

Divergence with Clojure CLI script when determining default config directory #114

Closed
DerGuteMoritz opened this issue Oct 11, 2023 · 7 comments · Fixed by #115
Closed

Divergence with Clojure CLI script when determining default config directory #114

DerGuteMoritz opened this issue Oct 11, 2023 · 7 comments · Fixed by #115

Comments

@DerGuteMoritz
Copy link
Contributor

As @jlesquembre figured out, there is a divergence with how the Clojure CLI shell script constructs the default directory. This is caused by difference in how the user home directory is determined: deps.clj uses the user.home JVM property while the shell script uses the HOME environment variable. This has potentially different results on macOS (see link).

@borkdude
Copy link
Owner

Can you please summarize the issue here so I don't have to dig into external links? Also it would save me time if you could make explicit what is the current + expected behavior and if there should be a change somewhere, possibly with multiple alternative solutions.

@DerGuteMoritz
Copy link
Contributor Author

DerGuteMoritz commented Oct 11, 2023

The summary is in the description already: Because deps.clj and the shell script use different mechanisms to look up the user home directory, they can end up with different results. Here you can see that the shell script uses $HOME while borkdude.deps/home-dir uses (System/getProperty "user.home") which is not equivalent (see e.g. JDK-8280357 for how this caused trouble in the past). The only fix I can see is to switch to (*getenv-fn* "HOME") in deps.clj, as well, to exactly match the shell script's behavior (which AFAIUI is something that deps.clj strives to do). Hope that helps!

@DerGuteMoritz
Copy link
Contributor Author

Oh yeah and we ran into this because the nix build environment on Darwin ends up with diverging results between the two in practice (see NixOS/nixpkgs#257473 (comment)).

@DerGuteMoritz
Copy link
Contributor Author

If you agree, I can cook up a PR for this later, too!

@borkdude
Copy link
Owner

Please double-check how the CLI checks for the home dir on Windows.

@borkdude
Copy link
Owner

@borkdude
Copy link
Owner

let's make the change, PR welcome

DerGuteMoritz added a commit to bevuta/deps.clj that referenced this issue Oct 11, 2023
The original bash script uses the `HOME` environment variable[1] to determine the user's home
directory while we were using thee `user.home` JVM property on non-windows platforms. This turns out
to not be equivalent (see e.g. JDK-8280357[2] for a recent issue caused by this). So instead, we now
also use the environment variable.

As it turns out, the original Powershell script also first checks the `HOME` environment variable[3]
before falling back to `USERPROFILE` (which is what we were using so far), so we also align that to
ensure equivalent behavior. This also means we can drop the "workaround" comment here since it no
longer is one.

Closes borkdude#114.

[1]  https://github.com/clojure/brew-install/blob/b03428b1b31a8d60573a284d6971fbd3db7b748d/src/main/resources/clojure/install/clojure#L280
[2]  https://bugs.openjdk.org/browse/JDK-8280357
[3]  https://github.com/clojure/brew-install/blob/dabb82bd011b0510006dd88464f350052adb69b5/src/main/resources/clojure/install/ClojureTools.psm1#L254-L258
DerGuteMoritz added a commit to bevuta/deps.clj that referenced this issue Oct 11, 2023
The original bash script uses the `HOME` environment variable[1] to determine the user's home
directory while we were using thee `user.home` JVM property on non-windows platforms. This turns out
to not be equivalent (see e.g. JDK-8280357[2] for a recent issue caused by this). So instead, we now
also use the environment variable.

As it turns out, the original Powershell script also first checks the `HOME` environment variable[3]
before falling back to `USERPROFILE` (which is what we were using so far), so we also align that to
ensure equivalent behavior. This also means we can drop the "workaround" comment here since it no
longer is one.

Closes borkdude#114.

[1]  https://github.com/clojure/brew-install/blob/b03428b1b31a8d60573a284d6971fbd3db7b748d/src/main/resources/clojure/install/clojure#L280
[2]  https://bugs.openjdk.org/browse/JDK-8280357
[3]  https://github.com/clojure/brew-install/blob/dabb82bd011b0510006dd88464f350052adb69b5/src/main/resources/clojure/install/ClojureTools.psm1#L254-L258
DerGuteMoritz added a commit to bevuta/deps.clj that referenced this issue Oct 12, 2023
The original bash script uses the `HOME` environment variable[1] to determine the user's home
directory while we were using thee `user.home` JVM property on non-windows platforms. This turns out
to not be equivalent (see e.g. JDK-8280357[2] for a recent issue caused by this). So instead, we now
also use the environment variable.

As it turns out, the original Powershell script also first checks the `HOME` environment variable[3]
before falling back to `USERPROFILE` (which is what we were using so far), so we also align that to
ensure equivalent behavior. This also means we can drop the "workaround" comment here since it no
longer is one.

Closes borkdude#114.

[1]  https://github.com/clojure/brew-install/blob/b03428b1b31a8d60573a284d6971fbd3db7b748d/src/main/resources/clojure/install/clojure#L280
[2]  https://bugs.openjdk.org/browse/JDK-8280357
[3]  https://github.com/clojure/brew-install/blob/dabb82bd011b0510006dd88464f350052adb69b5/src/main/resources/clojure/install/ClojureTools.psm1#L254-L258
DerGuteMoritz added a commit to bevuta/deps.clj that referenced this issue Oct 12, 2023
The original bash script uses the `HOME` environment variable[1] to determine the user's home
directory while we were using thee `user.home` JVM property on non-windows platforms. This turns out
to not be equivalent (see e.g. JDK-8280357[2] for a recent issue caused by this). So instead, we now
also use the environment variable.

As it turns out, the original Powershell script also first checks the `HOME` environment variable[3]
before falling back to `USERPROFILE` (which is what we were using so far), so we also align that to
ensure equivalent behavior. This also means we can drop the "workaround" comment here since it no
longer is one.

Closes borkdude#114.

[1]  https://github.com/clojure/brew-install/blob/b03428b1b31a8d60573a284d6971fbd3db7b748d/src/main/resources/clojure/install/clojure#L280
[2]  https://bugs.openjdk.org/browse/JDK-8280357
[3]  https://github.com/clojure/brew-install/blob/dabb82bd011b0510006dd88464f350052adb69b5/src/main/resources/clojure/install/ClojureTools.psm1#L254-L258
borkdude pushed a commit that referenced this issue Oct 12, 2023
The original bash script uses the `HOME` environment variable[1] to determine the user's home
directory while we were using thee `user.home` JVM property on non-windows platforms. This turns out
to not be equivalent (see e.g. JDK-8280357[2] for a recent issue caused by this). So instead, we now
also use the environment variable.

As it turns out, the original Powershell script also first checks the `HOME` environment variable[3]
before falling back to `USERPROFILE` (which is what we were using so far), so we also align that to
ensure equivalent behavior. This also means we can drop the "workaround" comment here since it no
longer is one.

Closes #114.

[1]  https://github.com/clojure/brew-install/blob/b03428b1b31a8d60573a284d6971fbd3db7b748d/src/main/resources/clojure/install/clojure#L280
[2]  https://bugs.openjdk.org/browse/JDK-8280357
[3]  https://github.com/clojure/brew-install/blob/dabb82bd011b0510006dd88464f350052adb69b5/src/main/resources/clojure/install/ClojureTools.psm1#L254-L258
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants