davidanthoff commented 3 years ago

This adds an option to extract a tarball without using any of the file permissions in the tarball.

I primarily need this on Windows so that I can extract a tarball and have all the extracted files just inherit permissions from the folder where I extract them to.

If this PR is acceptable I'll add a test and docs.

StefanKarpinski commented 3 years ago

The name same_permissions doesn't really suggest much of a meaning to me. It seems like this doesn't even try to set any kind of executable permissions on files that are supposed to be executable?

davidanthoff commented 3 years ago

The name same_permissions doesn't really suggest much of a meaning to me.

I just copied that name from the original tar command, where that is a command line flag that has the same effect as the flag in this PR, AFAIK. Happy to use a better suggestion if someone has one :)

It seems like this doesn't even try to set any kind of executable permissions on files that are supposed to be executable?

Yes, exactly, this just does not touch file ACLs at all, which is probably what one wants in the vast majority of cases on Windows. Permissions on Windows are in almost all cases handled at the container level and then inherited, and not set on a per-file basis. For example I'm not aware of a single installer technique on Windows - by MS or anyone else- that is setting individual file level permissions, permissions are always handled on some parent folder that constitutes some sort of security boundary and then inherited to all files in that folder. The equivalent of the executable flag that signals that something can be run is really the file extension on Windows.

I need a way to extract files from a tarball for https://github.com/julialang/juliaup that just does not set any file permissions on the extracted files, but has all the extracted files inherit file permissions from their parent folder.

StefanKarpinski commented 3 years ago

How about calling this set_permissions then?

davidanthoff commented 3 years ago

Yes, set_permissions is much nicer. I changed that, and also added a (pretty simplistic) test that should exercise the new code path and added some doc language.

davidanthoff commented 3 years ago

@StefanKarpinski if we manage to merge this before Tue it might still make it into Julia 1.7, right?

StefanKarpinski commented 3 years ago

Yeah, we can sneak this in. Looks good to me if you add tests and docs.

davidanthoff commented 3 years ago

I added a test. It is not super sophisticated, but maybe good enough? Kind of tricky to write a test that really tests permissions properly, because the new behavior is that it depends on the permissions of the parent and that depends on the system on which this is running... But the test I added does exercise the code path.

Docs are also there.

StefanKarpinski commented 3 years ago

Btw, once this gets merged I'll include it in Tar 1.10.0 and bump it on Julia master. I assume you don't need this on Julia 1.6.x — I don't think we can do that since this is clearly a feature that expands the API of the package.

codecov[bot] commented 3 years ago

Codecov Report

Merging #113 (78640d8) into master (fdeec31) will increase coverage by 0.78%. The diff coverage is 100.00%.

:exclamation: Current head 78640d8 differs from pull request most recent head 4919ece. Consider uploading reports for the commit 4919ece to get more accurate results

@@            Coverage Diff             @@
##           master     #113      +/-   ##
==========================================
+ Coverage   96.05%   96.84%   +0.78%     
==========================================
  Files           4        4              
  Lines         685      729      +44     
==========================================
+ Hits          658      706      +48     
+ Misses         27       23       -4

Impacted Files	Coverage Δ
src/Tar.jl	`92.77% <ø> (ø)`
src/extract.jl	`97.97% <100.00%> (+1.38%)`	: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 fdeec31...4919ece. Read the comment docs.

StefanKarpinski commented 3 years ago

I've made the set_permissions tests run on all platforms since this is not a Windows-specific feature — it's available on all platforms and needs to be tested along with the rest of the Tar.extract API.

StefanKarpinski commented 3 years ago

Update will be included in Julia 1.7 once this PR is merged: https://github.com/JuliaLang/julia/pull/41129.

JuliaIO / Tar.jl

Add set_permissions argument #113

Codecov Report