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

Use atomic writes when binding artifacts #3235

Merged
merged 2 commits into from
Oct 24, 2022
Merged

Use atomic writes when binding artifacts #3235

merged 2 commits into from
Oct 24, 2022

Conversation

staticfloat
Copy link
Member

@staticfloat staticfloat commented Oct 24, 2022

EDIT: No longer doing this, Jameson has Shown Me The Way™

We need to clear this cache when modifying the TOML file. This should fix #3212.

It might be nice to expose a Base.drop_parsed_toml(path::String) API so that I don't have to manually reach into TOML_CACHE.d here like this.

@staticfloat staticfloat requested a review from vtjnash October 24, 2022 16:23
src/Artifacts.jl Outdated
Comment on lines 226 to 229
open(artifacts_toml, "w") do io
TOML.print(io, artifact_dict, sorted=true)
end
Copy link
Member

Choose a reason for hiding this comment

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

Ah, these lines are buggy, causing this failure. Pkg must never open an existing file for writing, as it causes problems for the filesystem.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@KristofferC KristofferC Oct 24, 2022

Choose a reason for hiding this comment

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

I think JuliaLang/julia#46944 removed the check that was intended to catch things like this.

Copy link
Member

Choose a reason for hiding this comment

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

It did, but that check was fundamentally flawed, and didn't actually check for things like this, it merely checked for attempts to test for this problem.

@staticfloat staticfloat changed the title Drop Base TOML cache when binding artifacts Use atomic writes when binding artifacts Oct 24, 2022
This ensures that the inode of the underlying `Artifacts.toml` file gets
changed, which should properly invalidate Base's TOML cache.
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Excellent

@IanButterworth
Copy link
Member

Might as well shoot for all green given the stdlib check is fixed on master now

@IanButterworth
Copy link
Member

Made an issue for the MacOS error which appears unrelated but has been sticking around #3238

Also the historical stdlib check is now removed as of #3096

@IanButterworth IanButterworth merged commit 6d0c62a into master Oct 24, 2022
@IanButterworth IanButterworth deleted the sf/fix_3212 branch October 24, 2022 22:07
@simeonschaub
Copy link
Member

simeonschaub commented Apr 5, 2023

@staticfloat This broke support for relative Artifact.toml paths in JuliaPackaging/ArtifactUtils.jl#19. Should this be fixed here with something like:

diff --git a/src/Artifacts.jl b/src/Artifacts.jl
index 538755e23..dca06e469 100644
--- a/src/Artifacts.jl
+++ b/src/Artifacts.jl
@@ -223,7 +223,8 @@ function bind_artifact!(artifacts_toml::String, name::String, hash::SHA1;
 
     # Spit it out onto disk
     let artifact_dict = artifact_dict
-        temp_artifacts_toml = tempname(dirname(artifacts_toml))
+        parent_dir = dirname(artifacts_toml)
+        temp_artifacts_toml = isempty(parent_dir) ? : tempname(pwd()) : tempname(parent_dir)
         open(temp_artifacts_toml, "w") do io
             TOML.print(io, artifact_dict, sorted=true)
         end

or was this never a supported use case and ArtifactUtils should be updated?

@staticfloat
Copy link
Member Author

A patch like yours should be fine.

simeonschaub added a commit that referenced this pull request Apr 5, 2023
simeonschaub added a commit that referenced this pull request Apr 5, 2023
KristofferC pushed a commit that referenced this pull request Apr 11, 2023
IanButterworth pushed a commit to IanButterworth/Pkg.jl that referenced this pull request Apr 11, 2023
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.

Artifact hash errors on nightly 64-bit
5 participants