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

Add set_permissions argument #113

Merged
merged 8 commits into from
Jun 8, 2021

Conversation

davidanthoff
Copy link
Contributor

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

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

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

How about calling this set_permissions then?

@davidanthoff davidanthoff marked this pull request as ready for review May 28, 2021 21:46
@davidanthoff
Copy link
Contributor Author

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 davidanthoff changed the title Add same_permissions argument Add set_permissions argument May 29, 2021
@davidanthoff
Copy link
Contributor Author

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

@StefanKarpinski
Copy link
Member

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

@davidanthoff
Copy link
Contributor Author

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.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

Seems to lax not to test permissions at all. We can at least test the resulting permissions on all non-Windows systems since those are predictable. And we can test that f_path and x_path have the same resulting permissions on all platforms.

test/runtests.jl Outdated Show resolved Hide resolved
test/runtests.jl Outdated Show resolved Hide resolved
@StefanKarpinski
Copy link
Member

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

codecov bot commented Jun 8, 2021

Codecov Report

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

❗ Current head 78640d8 differs from pull request most recent head 4919ece. Consider uploading reports for the commit 4919ece to get more accurate results
Impacted file tree graph

@@            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%) ⬆️

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

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.

src/Tar.jl Outdated Show resolved Hide resolved
@StefanKarpinski StefanKarpinski merged commit e02d5be into JuliaIO:master Jun 8, 2021
@StefanKarpinski
Copy link
Member

Update will be included in Julia 1.7 once this PR is merged: JuliaLang/julia#41129.

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