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

Avoid deleting existing artifacts when ignoring hashes. #3768

Merged
merged 3 commits into from
Jan 23, 2024

Conversation

nhz2
Copy link
Contributor

@nhz2 nhz2 commented Jan 23, 2024

Followup to #3764
Fixes bug reported in #3643 (comment)

This PR modifies download_artifact to avoid calling mv(src, dst; force=true) where src and dst are both possible valid artifacts.

The main steps of download_artifact are

  1. Return true if the artifact already exists.
  2. Determine the expected artifact path and parent directory.
  3. Make a temporary directory.
  4. download_verify_unpack into the temporary directory.
  5. Calculate the tree hash of the temporary directory.
  6. If the tree hash doesn't match, either throw an error or log an error, depending on the enviroment.
  7. Atomically move the temporary directory to the expected path.
  8. Try to clean up the temporary directory.

If steps 4 to 7 throw an error, that error gets returned, otherwise true is returned.

@nhz2 nhz2 marked this pull request as ready for review January 23, 2024 03:48
@IanButterworth
Copy link
Member

@staticfloat does this look ok to you?

@IanButterworth
Copy link
Member

Given you have a reproducer in #3643 (comment) could that be added as a test?

@nhz2
Copy link
Contributor Author

nhz2 commented Jan 23, 2024

Yes, I'll try and add a test. I should be able to reproduce the issue on Linux using the right environment variables and artifacts.

@IanButterworth IanButterworth merged commit a83783e into JuliaLang:master Jan 23, 2024
13 checks passed
IanButterworth pushed a commit that referenced this pull request Jan 23, 2024
* avoid deleting existing artifacts

* fix order

* add testset

(cherry picked from commit a83783e)
KristofferC pushed a commit that referenced this pull request May 9, 2024
* avoid deleting existing artifacts

* fix order

* add testset
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 this pull request may close these issues.

3 participants