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

Always use the ustar format of tar #73

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

choroba
Copy link

@choroba choroba commented Feb 10, 2020

This should prevent the module from being uninstallable on systems
that don't support PAX headers, but the tar of the creator of the
distribution supports them.

See e.g. Dual-Life/Devel-PPPort#183 or
https://huntingbears.nl/2015/02/17/new-tar-paxheaders-and-installing-from-cpan/

This should prevent the module from being uninstallable on systems
that don't support PAX headers, but the tar of the creator of the
distribution supports them.

See e.g. Dual-Life/Devel-PPPort#183 or
https://huntingbears.nl/2015/02/17/new-tar-paxheaders-and-installing-from-cpan/
@Grinnz
Copy link
Collaborator

Grinnz commented Feb 10, 2020

Is this portable?

@choroba
Copy link
Author

choroba commented Feb 10, 2020

I have no idea :-( but it probably isn't

@pali
Copy link

pali commented Feb 13, 2020

It looks like that GNU tar and BSD tar supports -o parameter which should create ustar archive when combined together with -c.

@@ -440,6 +440,7 @@ my %WriteMakefileArgs = (
#'ABC' => '1.6',
#'Foo::Bar::Module' => '5.0401',
},
macro => { TARFLAGS => '--format=ustar -c -v -f' },
dist => { COMPRESS => 'gzip -9f', SUFFIX => 'gz', },
Copy link

Choose a reason for hiding this comment

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

According to ExtUtils::MakeMaker documentation, TARFLAGS should be in dist key, not in macro key.

Copy link
Author

Choose a reason for hiding this comment

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

Interesting, it seems to work both ways. Also, I'm not sure what I originally read about BSD, it seems its tar supports --format ustar as well as -o.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The dist section is options for creating the distribution tarball which is what we want here. macro is undocumented so we would need an archaeologist to find its purpose.

Copy link

Choose a reason for hiding this comment

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

macro is general purpose, dist is for building tarballs specifically. macro happens to work because it happens to be defined after dist, but this shouldn't be relied upon.

@haarg
Copy link

haarg commented Feb 13, 2020

Normally, tar on MacOS won't use PAX headers. However, if some file attributes aren't possible to represent in a standard ustar format, it will add the PaxHeaders. This happens on one of my machines due to having a uid and gid that are too large. However, specifying --format=ustar just means that those files don't get stored in the tarball at all. I haven't found a workaround for this aside from using GNU tar, which lets you specify the uid and gid.

I'm not certain what happened to cause the PAX headers in the Devel-PPPort case.

@choroba
Copy link
Author

choroba commented Feb 13, 2020

Updated with a fixup. Maybe we can first run tar -o to try whether it's supported and only then add the option?

@Grinnz
Copy link
Collaborator

Grinnz commented Feb 13, 2020

Remember that this is not Module::Starter's Makefile.PL, it is a template for any distribution created with Module::Starter, that is then expected to be maintained by the author. I am not in favor of adding anything complex.

@choroba
Copy link
Author

choroba commented Feb 14, 2020

@Grinnz I know. I wanted to prevent new distributions from using PAX headers and failing to install randomly.

@pali
Copy link

pali commented Feb 14, 2020

Seems that GNU tar -o is not same as --format=ustar. GNU tar manpage about -o says: When creating, same as --old-archive. And for --old-archive: Same as --format=v7. --format=v7 is documented as: Old V7 tar format. And --format=ustar as POSIX 1003.1-1988 (ustar) format.

On the other hand BSD tar -o has documented it as: A synonym for --format ustar.

So to enforce ustar on both GNU tar and BSD tar, it is needed to use --format=ustar.

@haarg
Copy link

haarg commented Feb 14, 2020

In case my previous comment wasn't clear: specifying the format will not cause bsd tar (at least on MacOS) to create correct archives where it was previously creating bad archives. Instead, it will cause it to create empty archives.

@choroba
Copy link
Author

choroba commented Feb 14, 2020

What about switching to ptar from Archive::Tar? It's been core since 5.10 and seems to create POSIX archives without PAX headers. So, we could just set

dist => {
    TAR => 'ptar',

@choroba
Copy link
Author

choroba commented Feb 14, 2020

@haarg: Does ptar create the archive when bsd tar creates an emtpy archive?

@haarg
Copy link

haarg commented Feb 14, 2020

Archive::Tar/ptar will create a normal archive.

@Grinnz
Copy link
Collaborator

Grinnz commented Feb 14, 2020

A solution requiring 5.10 is tricky. Even if I set the initial minimum to 5.10 (I would consider 5.8), there is nothing keeping the author from then lowering it, and having a broken dist.

@pali
Copy link

pali commented Feb 14, 2020

That module is in Core since 5.10. But it does not mean you cannot install it also on previous version. Also it is needed only when creating new distribution release, not when installing. It is really a problem?

@Grinnz
Copy link
Collaborator

Grinnz commented Feb 14, 2020

Module::Starter has a unique role in the templates it provides. They become accidental best practice and cargo cult. The contents usually end up sticking around not only through the whole distribution but also when it is handed off to other authors. We should take care in anything we introduce to them, that it is clear why everything is needed, and that it does not cause more problems than it helps.

We would have much more freedom in, say, a Dist::Zilla minting profile, where its usage is specific. But here we are essentially handing out abacuses. I would prefer to keep it as footgun free as possible.

@Grinnz
Copy link
Collaborator

Grinnz commented Feb 14, 2020

Sadly a configure requires would not be appropriate here, even though the template already has the compatibility code for that. This would be a develop phase prereq, which Makefile.PL itself can't handle.

@choroba
Copy link
Author

choroba commented Feb 14, 2020

Maybe we should change ExtUtils::MakeMaker to use ptar if available instead?

@Leont
Copy link

Leont commented Feb 16, 2020

Maybe we should change ExtUtils::MakeMaker to use ptar if available instead?

Honestly, the last thing MakeMaker needs is new responsibilities.

@pali
Copy link

pali commented Feb 21, 2020

What do you mean by a new responsibilities?

It is already ExtUtils::MakeMaker who generates Makefile. And if that generated Makefile is wrong, i.e. cause that make dist creates a problematic distribution archive then it is a problem of code which generated that Makefile -- ExtUtils::MakeMaker.

So ExtUtils::MakeMaker is already responsible for creating distribution archive, so it should do it correctly.

@Leont
Copy link

Leont commented Feb 21, 2020

What do you mean by a new responsibilities?

Figuring out "do we have a working ptar" or "do we have a working gnu tar", and in particular doing so portably.

@xsawyerx
Copy link
Owner

Module-Starter is used as-is for a very long time until this issue came up.

We can simply decide on the exact situation in which we're changing the default ($^O =~ /aix/i for example) and the rest would be the current situation.

@choroba
Copy link
Author

choroba commented Apr 13, 2020

@xsawyerx That's because tar changed its behaviour. It started adding the PAX headers by default.

@haarg
Copy link

haarg commented Apr 16, 2020

It's not clear to me why Devel::PPPort ended up with PAX headers, but adding them isn't the default on any system that I know of, aside from when the file metadata can't be stored in ustar format.

@choroba
Copy link
Author

choroba commented Apr 16, 2020

@haarg When I run tar on my OpenSUSE Leap 15.1, the check from Devel-PPPort says it's pax:

~ $ tar cf all.tar *.pl
~ $ grep PaxHeader all.tar
Binary file all.tar matches

(GNU tar) 1.30

@pali
Copy link

pali commented Apr 16, 2020

Hi @haarg!

It's not clear to me why Devel::PPPort ended up with PAX headers

That is because new version of GNU /usr/bin/tar switched to PAX format by default.

but adding them isn't the default on any system that I know of

Now it default on Fedora, OpenSUSE Leap and probably any rolling-edge linux distribution. And I guess this change would propagate to next SLES, RHEL or Debian stable versions too.

@Grinnz
Copy link
Collaborator

Grinnz commented Apr 16, 2020

I have released multiple distributions with tar 1.32 on Fedora 30 and it does not add the headers. Are you sure it is not just adding them because of something the ustar format can't store, like before?

@pali
Copy link

pali commented Apr 16, 2020

That is interesting. Maybe it is really because of something which ustar cannot store. In any case it is happening which means /usr/bin/tar is doing it and therefore Makefile should be fixed to not generate PAXed tarballs.

@Grinnz
Copy link
Collaborator

Grinnz commented Apr 16, 2020

If it is something ustar can't store, then forcing it to ustar means those will not get stored in the tarball at all, as @haarg said earlier in the ticket.

@choroba
Copy link
Author

choroba commented Apr 16, 2020

Please, download tars.zip from my site and tell me what the tar.tar stores in the pax headers. It's missing in the other two, ptar.tar and ustar.tar. Is it something we need to store in a distribution?
tar.tar was created using tar, ustar.tar was created using tar c -H ustar and ptar.tar was created using ptar.

@haarg
Copy link

haarg commented Apr 18, 2020

It's storing ctime, mtime, and atime in the pax headers. Pax headers allow for high resolution times than ustar, which may be why they are being used.

The problematic Devel-PPPort release was only storing mtime, and only for the Changes and Makefile.PL files.

For this case, specifying --format=ustar should solve the issue, because tar will just store the less accurate time.

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.

6 participants