Skip to content

Conversation

websurfer5
Copy link
Contributor

@websurfer5 websurfer5 commented Jun 2, 2019

As a result of the current tarfile behavior, shutil.make_archive (xxx, tar, root_dir) results in a './' entry in the archive, which is wrong.

@brettcannon brettcannon added the type-bug An unexpected behavior, bug, or error label Jun 3, 2019
@taleinat taleinat changed the title bpo-35964: sshutil.make_archive (xxx, tar, root_dir) is adding './' entry to archive which is wrong bpo-35964: shutil.make_archive (xxx, tar, root_dir) is adding './' entry to archive which is wrong Jun 29, 2019
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

I don't think the change to shutil warrants a mention in "What's New". The similar fix for zip archives did not add such a mention. The "What's New" line about the change to tarfile is fine IMO, though.

Also, since this changes the tarfile module (rather than shutil), this also should be tested in the tar module's tests.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@taleinat taleinat requested a review from gustaebel July 11, 2019 13:40
Copy link
Contributor

@taleinat taleinat left a comment

Choose a reason for hiding this comment

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

Looks good to me.

This should also be reviewed by @gustaebel, the expert on the tarfile module.

@arhadthedev arhadthedev changed the title bpo-35964: shutil.make_archive (xxx, tar, root_dir) is adding './' entry to archive which is wrong gh-80145: shutil.make_archive (xxx, tar, root_dir) is adding './' entry to archive which is wrong May 18, 2023
@arhadthedev arhadthedev added 3.11 only security fixes 3.12 only security fixes needs backport to 3.11 only security fixes and removed 3.11 only security fixes 3.12 only security fixes labels May 18, 2023
@iritkatriel iritkatriel added the needs backport to 3.12 only security fixes label Jun 21, 2023
@terryjreedy terryjreedy removed the needs backport to 3.11 only security fixes label Apr 21, 2024
@terryjreedy terryjreedy changed the title gh-80145: shutil.make_archive (xxx, tar, root_dir) is adding './' entry to archive which is wrong gh-80145: Exclude '.' directory when adding directories to a tar archive Apr 21, 2024
@terryjreedy
Copy link
Member

terryjreedy commented Apr 21, 2024

I changed the title to reflect what the patch does, as opposed the the symptom it prevents. This raises the question of whether it is always wrong for tarfile to add './', so that this is truly a bug fix, or only wrong when tarfile is called from shutil? In other words, is the bug possibly in the arguments passed by shutil, or tarfile not interpreting them fully? Tal Einat's approval of this PR was conditioned on an answer from a 'tarfile expert'. Lars is no longer reviewing. @serhiy-storchaka Can you decide this?

Otherwise, since the tests still pass after merging main, I think this good to go. The OP changed test_tarfile as Tal requested. I removed the 3.11 backport label and updated a comment from 'bpo' to 'gh'.

@gvanrossum
Copy link
Member

Well, it would leave out the permissions ("mode") and other metadata for the current directory, which might be used when extracting (possibly using a different tar implementation) into a specific directory that doesn't exist yet but which that utility might create, setting the mode from the tarchive? So it seems to me this might not be 100% backwards compatible.

@nineteendo
Copy link
Contributor

Isn't that the same as for zip archives? #72674

@serhiy-storchaka
Copy link
Member

I am not sure that we should change tarfile.add(), currently it is consistent with the tar command which adds the '.' entry if it is explicitly specified. Also, while this PR removes the '.' entry, it leaves the './' prefix in other entries.

I created an alternate PR #118152 which only changes shutil.make_archive(). You can restore the old behavior if explicitly specify base_dir='.'.

@gvanrossum
Copy link
Member

Agreed, tar itself does this too. What's special about shutil.make_archive() that it should be different? I'd prefer to be consistent and just reject the original issue.

@gvanrossum gvanrossum closed this Apr 22, 2024
@serhiy-storchaka
Copy link
Member

It is special because we have no way to create a tar file without the '.' entry and the ./ prefix. With the tar utility we can at least call tar * instead of tar ..

@gvanrossum
Copy link
Member

Surely we can do the equivalent of * using the glob module?

@serhiy-storchaka
Copy link
Member

os.listdir() is simpler. We can do this inside of shutil.make_archive(), currently the user of shutil.make_archive() cannot affect this. This is what #118152 does -- changes the default behavior (to presumably more useful), while leaving the possibility to get the old result if the user really needs it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting merge needs backport to 3.12 only security fixes type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.