JuliaIO / Tar.jl

TAR files: create, list, extract them in pure Julia
MIT License
79 stars 19 forks source link

Windows file modes: turn executable bits off for plain fails #83

Closed StefanKarpinski closed 3 years ago

StefanKarpinski commented 3 years ago

Unbekownst to me, Windows defaults to files being executable, so we were incorrectly extracting normal files as executables. We didn't detect this because until https://github.com/JuliaLang/julia/pull/35625 we were blind to the executable bit on Windows. With that change, however, we can now tell that we are incorrectly leaving normal files executable. https://github.com/JuliaLang/Pkg.jl/pull/2253 fixes Pkg's GitTools.tree_hash and in the process breaks our tests since they now correctly detect that we are extracting non-executable files incorrectly on Windows. This PR fixes that, making tests pass again with that fix. Failure observed in https://github.com/JuliaLang/julia/pull/38678.

codecov[bot] commented 3 years ago

Codecov Report

Merging #83 (a47bbd0) into master (74a2e67) will increase coverage by 0.69%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #83      +/-   ##
==========================================
+ Coverage   95.68%   96.37%   +0.69%     
==========================================
  Files           4        4              
  Lines         579      635      +56     
==========================================
+ Hits          554      612      +58     
+ Misses         25       23       -2     
Impacted Files Coverage Δ
src/extract.jl 96.27% <100.00%> (+1.08%) :arrow_up:
src/header.jl 94.02% <100.00%> (+0.69%) :arrow_up:
src/create.jl 97.05% <0.00%> (+0.14%) :arrow_up:
src/Tar.jl 97.36% <0.00%> (+0.39%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 74a2e67...a47bbd0. Read the comment docs.

StefanKarpinski commented 3 years ago

So far, so good. Still need to try this in the problematic combination.

StefanKarpinski commented 3 years ago

As I suspect (and I'm impressed that the tests caught this!), the permission copy needed to be recursive, so I implemented that. There was another failure with this fix on Windows nightly which was a bit of a catch-22: since we were using Pkg.GitTools.tree_hash to compute on-disk tree hashes and that is still wrong on nightly, since this change creates tarballs with the correct executable permissions and then computes a correct tree hash for that tarball, it was disagreeing with Pkg.GitTools.tree_hash and failing tests. Since we can't merge the Pkg fix until we make the Tar tests pass, we're in a bit of a bind. To escape, I changed the tests to use a copy of GitTools with Windows tree hash fix already applied. We could potentially revert that bit once the fix to Pkg is merged on nightly, but it seems better to just have a copy of that tree hashing implementation here since we're likely to switch Pkg to using Tar.tree_hash in the near future anyway and then we could potentially just delete Pkg.GitTools.tree_hash.