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

ignore tree hashes on Windows w/o symlink capability #3764

Merged
merged 4 commits into from
Jan 22, 2024

Conversation

StefanKarpinski
Copy link
Member

Step one of addressing #3643. I implemented this by providing a default for the ignore_hash value in download_artifact, namely the default value is false unless the OS is Windows and the user cannot create symlinks in the artifact directory, in which case we default ignore_hash to true instead. This still emits a warning, which seems desirable based on the discussion in #3643.

staticfloat
staticfloat previously approved these changes Jan 17, 2024
src/Artifacts.jl Outdated
Comment on lines 324 to 338
ignore_hash = if get(ENV, "JULIA_PKG_IGNORE_HASHES", "") == ""
# default: false except Windows users who can't symlink
Sys.iswindows() && !mktempdir(can_symlink, dirname(src))
else
Base.get_bool_env("JULIA_PKG_IGNORE_HASHES", ignore_hash)
ignore_hash === nothing && @error(
"Invalid ENV[\"JULIA_PKG_IGNORE_HASHES\"] value",
ENV["JULIA_PKG_IGNORE_HASHES"],
)
ignore_hash = something(ignore_hash, false)
end
Copy link
Member

@IanButterworth IanButterworth Jan 17, 2024

Choose a reason for hiding this comment

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

I think this does what you want?

Suggested change
ignore_hash = if get(ENV, "JULIA_PKG_IGNORE_HASHES", "") == ""
# default: false except Windows users who can't symlink
Sys.iswindows() && !mktempdir(can_symlink, dirname(src))
else
Base.get_bool_env("JULIA_PKG_IGNORE_HASHES", ignore_hash)
ignore_hash === nothing && @error(
"Invalid ENV[\"JULIA_PKG_IGNORE_HASHES\"] value",
ENV["JULIA_PKG_IGNORE_HASHES"],
)
ignore_hash = something(ignore_hash, false)
end
# default: false except Windows users who can't symlink
default = Sys.iswindows() && !mktempdir(can_symlink, dirname(src))
ignore_hash = Base.get_bool_env("JULIA_PKG_IGNORE_HASHES", default)

Copy link
Member Author

@StefanKarpinski StefanKarpinski Jan 17, 2024

Choose a reason for hiding this comment

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

That's what I had at first, but there are two issues:

  1. I don't want to even try to make a symlink if the variable is set.
  2. If the variable is set but has an invalid value, I don't want this code to inscrutably error.

The former would be addressed if the API to Base.get_bool_env allowed a callback to get a default value that is only evaluated if needed (like get). The latter would be addressed if Base.get_bool_env raised an error on invalid value instead of returning nothing.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable JuliaLang/julia#52950

src/Artifacts.jl Outdated Show resolved Hide resolved
@StefanKarpinski StefanKarpinski force-pushed the sk/ignore-hashes-windows-wo-symlinks branch from 83b3449 to 736ac32 Compare January 18, 2024 18:21
@StefanKarpinski
Copy link
Member Author

If someone wants to help me debug this, I'm slammed and haven't and won't have the time (kid was sick last week, I was sick over the weekend, childcare is sick now, plus have to work), but this needs to go into 1.10.1. An intermediate version did work, now it's failing tests, probably because the tests expect some specific error message. If someone could help track down why this is failing now, that would really help.

@IanButterworth

This comment was marked as resolved.

@IanButterworth
Copy link
Member

Ok. The logic was just inverted on ignore_hash_env_set. This passes artifacts tests locally now

@IanButterworth IanButterworth merged commit b431875 into master Jan 22, 2024
13 checks passed
@IanButterworth IanButterworth deleted the sk/ignore-hashes-windows-wo-symlinks branch January 22, 2024 18:16
IanButterworth pushed a commit that referenced this pull request Jan 22, 2024
* ignore tree hashes on Windows w/o symlink capability

Step one of addressing #3643

* avoid test setting ENV to "nothing"

* Revert "avoid test setting ENV to "nothing""

This reverts commit 5d98110.

* fix logic on ignore_hash_env_set

---------

Co-authored-by: Ian Butterworth <[email protected]>
(cherry picked from commit b431875)
@StefanKarpinski
Copy link
Member Author

Thank you so much, @IanButterworth! You're the best ❤️

vtjnash pushed a commit to JuliaLang/julia that referenced this pull request Jan 31, 2024
KristofferC pushed a commit that referenced this pull request May 9, 2024
* ignore tree hashes on Windows w/o symlink capability

Step one of addressing #3643

* avoid test setting ENV to "nothing"

* Revert "avoid test setting ENV to "nothing""

This reverts commit 5d98110.

* fix logic on ignore_hash_env_set

---------

Co-authored-by: Ian Butterworth <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants