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

PlatformEngines: use Tar.jl for tarball operations #2147

Merged
merged 1 commit into from
Oct 24, 2020

Conversation

StefanKarpinski
Copy link
Member

@StefanKarpinski StefanKarpinski commented Oct 21, 2020

The latest installment in my quest to make the name "PlatformEngines" increasingly nonsensical. As a practical matter, Tar.jl sets permissions correctly when extracting tarballs, which 7z does not, so this fixes an actual bug. This PR doesn't implement it but Tar.tree_hash will also allow us to reliably verify tree hashes before extracting tarballs, so we can actually protect against incorrect tarball content, even on broken file systems.

@StefanKarpinski StefanKarpinski force-pushed the sk/PlatformEngines3 branch 2 times, most recently from ba668b5 to 7f415f1 Compare October 21, 2020 22:13
@StefanKarpinski
Copy link
Member Author

Hard to tell if the AppVeyor failures are because I broke something or just network flakiness.

@StefanKarpinski
Copy link
Member Author

I don't believe that the AppVeyor CI failure is caused by this change, I think it's caused by JuliaLang/julia#38070.

@StefanKarpinski StefanKarpinski merged commit 827a740 into master Oct 24, 2020
@StefanKarpinski StefanKarpinski deleted the sk/PlatformEngines3 branch October 24, 2020 01:00
@KristofferC
Copy link
Member

What was the Windows problem in the end?

@@ -888,19 +365,10 @@ end
Compress `src_dir` into a tarball located at `tarball_path`.
"""
function package(src_dir::AbstractString, tarball_path::AbstractString)
Copy link
Member

Choose a reason for hiding this comment

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

This is also test only, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure, I thought the Artifacts code used it too, but I could be wrong.

@yuyichao
Copy link

This broke system 7z build!

@KristofferC
Copy link
Member

#2181

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