Align home-dir with original scripts
#115
Conversation
|
Can't think of a way to construct a regression test for this, alas 😕 |
|
Hm can't reproduce the test failures locally with babashka v1.3.184 🤔 There's some odd output: Maybe a flake? I can't trigger a re-run to try, though. |
|
Since all builds are failing, it doesn't look like a flake to me ;) |
deps.bat
Outdated
| ;; workaround for https://github.com/oracle/graal/issues/1630 | ||
| (*getenv-fn* "userprofile") | ||
| (System/getProperty "user.home"))) | ||
| (or (*getenv-fn* "home") |
There was a problem hiding this comment.
This should probably be capitalized: HOME.
Similar to USERPROFILE. Environment variables are case sensitive.
There was a problem hiding this comment.
Ah, you got a point! I just blindly copied the lowercase spelling from the userprofile in the windows branch. Apparently there, environment variables are case insensitive? 🤔 Anyhow, I also uppercased them there for consistency with the original script. Let's see how it goes 🤞
There was a problem hiding this comment.
as in the truthy branch of the (if windows? ...) form.
Hold on, I forgot to re-generate deps.clj and deps.bat... coming up.
There was a problem hiding this comment.
I don't know what you are referring to, maybe you can paste a Github permalink with a line number.
There was a problem hiding this comment.
It's the lines this patch is touching: https://github.com/borkdude/deps.clj/pull/115/files#diff-de1d8b28c217e59dca32b58323dd56ab342bbcb074663d89807109756be86dfdR263-R265 - as you can see, userprofile used to be lowercased before.
DerGuteMoritz
force-pushed
the
align-home-dir-with-original-scripts
branch
from
5aa4ab4 to
97138ed
Compare
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
force-pushed
the
align-home-dir-with-original-scripts
branch
from
97138ed to
28322dd
Compare
The original bash script uses the
HOMEenvironment variable[1] to determine the user's home directory while we were using theeuser.homeJVM 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
HOMEenvironment variable[3] before falling back toUSERPROFILE(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
Please answer the following questions and leave the below in as part of your PR.
This PR corresponds to an issue with a clear problem statement.
This PR contains a test to prevent against future regressions
I have updated the CHANGELOG.md file with a description of the addressed issue.