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

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

Merged
merged 5 commits into from
Dec 4, 2020

Conversation

StefanKarpinski
Copy link
Member

@StefanKarpinski StefanKarpinski commented Dec 4, 2020

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 JuliaLang/julia#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. JuliaLang/Pkg.jl#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 JuliaLang/julia#38678.

@codecov
Copy link

codecov bot commented Dec 4, 2020

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%) ⬆️
src/header.jl 94.02% <100.00%> (+0.69%) ⬆️
src/create.jl 97.05% <0.00%> (+0.14%) ⬆️
src/Tar.jl 97.36% <0.00%> (+0.39%) ⬆️

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
Copy link
Member Author

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

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 JuliaLang/julia#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.
JuliaLang/Pkg.jl#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.
@StefanKarpinski
Copy link
Member Author

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.

@StefanKarpinski StefanKarpinski merged commit 1b63f2a into master Dec 4, 2020
@StefanKarpinski StefanKarpinski deleted the sk/win-exec-fix branch December 4, 2020 15:52
KristofferC added a commit to KristofferC/Tar.jl that referenced this pull request Dec 5, 2020
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.

2 participants