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

zstd: make bottle relocatable #75880

Closed
wants to merge 1 commit into from
Closed

Conversation

carlocab
Copy link
Member

See #75458.


  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

@carlocab
Copy link
Member Author

carlocab commented Apr 25, 2021

Error: 1 failed step!
brew bottle --verbose --json zstd --only-json-tab

The bottle error is

Error: Operation already in progress for zstd.formula
Another active Homebrew process is already using zstd.formula.
Please wait for it to finish or terminate it to continue.
Error: Failure while executing; `/usr/local/bin/brew install --formula libarchive` exited with 1.

CC @MikeMcQuaid

Not sure why that process is still running though. Was it just not cleaned up properly?

@carlocab
Copy link
Member Author

carlocab commented Apr 26, 2021

brew bottle error also shows up at #75515.

I guess one way out here is for test-bot to just install libarchive before doing anything else (or perhaps this can be done in the workflow) on Mojave.

Not sure if that's what we want to do though.

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

Not sure why that process is still running though. Was it just not cleaned up properly?

Ah - interesting one! There's a lock on zstd while it is being bottled, but when we try to install libarchive it will try install zstd as it is a dependency.

Honestly, suggesting libarchive when it has a dependency tree like that is a bit of an oversight by myself. Having zstd lying about is also a opportunistic linkage nightmare (it's becoming quite regularly used).

It would be unusual, but it might be simpler for us to create an independent minimal bsdtar formula. It would be redundant for end users since libarchive will also contain it and would only really exist for internal reasons, but it would fit our process a bit better. It would also have less features than system tar, so I'm very hesitant to even have it exist in homebrew-core - it's a bit of an annoying situation.

@carlocab
Copy link
Member Author

Aha, yes, I see. So installing libarchive before doing anything won't help either.

It would be unusual, but it might be simpler for us to create an independent bsdtar formula. It would be redundant for end users since libarchive will also contain it, but it would fit our process a bit better.

Not opposed to this. Or, maybe even portable-bsdtar, so that we can stash it in HOMEBREW_LIBRARY where build systems are unlikely to find it. I've seen some build systems start looking for stuff in /usr/local/opt on macOS even when you don't want them to.

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

Time to bust out Gem::Package::TarWriter? 😄

@carlocab
Copy link
Member Author

carlocab commented Apr 26, 2021

I've also been seeing errors like this over a number of different formulae, but only on Mojave:

==> /usr/local/Cellar/devspace/5.12.1/bin/devspace help
/usr/local/Cellar/devspace/5.12.1/bin/devspace: /usr/local/Cellar/devspace/5.12.1/bin/devspace: cannot execute binary file

Wonder if it could be related to tarring with a new bsdtar but untarring with an old one. (Or perhaps more generally, just related to changes to brew bottle).

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

I'm surprised it would affect anything like that given any tar writer can't really break compatibility with older/other tar readers. Link the PR and I'll investigate after some sleep.

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

Decided to just look at it now instead.

Looks like sudo purge bug? 👀

00000000: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000010: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000020: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000030: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000040: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000050: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000060: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000070: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000080: 0000 0000 0000 0000 0000 0000 0000 0000  ................
00000090: 0000 0000 0000 0000 0000 0000 0000 0000  ................

I find that very interesting. We thought it was a Catalina bug but it seems like using a newer libarchive on Mojave also triggers it!

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

This is going to get me very interested in what changed in the libarchive code to trigger the bug.

@carlocab
Copy link
Member Author

Happy 🐛 hunting!

@MikeMcQuaid
Copy link
Member

@carlocab @Bo98 sorry, late to this party: can one of you summarise the current status/problem and I'll take a look?

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

Basically:

  • libarchive has a dependency tree that would be nice to avoid installing. This was an oversight by myself.
    • It pulls in zstd which is an opportunistic linkage nightmare. It also causes problems when we are bottling any of these dependencies due to locking.
    • It's not clear the best way to fix it - we could create a separate formula which is minimal and only has what we need, but I'm very hesitant to include internal-only formulae in homebrew-core.
    • There's plain Ruby ways to write tars (Gem::Package::TarWriter/minitar) though the current implementations don't implement things like xattr support, but could be extended upon if necessary.
  • We have discovered that the sudo purge bug exists on Mojave when using the newer bsdtar.
    • This is probably because of the sparse file support added in libarchive 3.0.

@MikeMcQuaid
Copy link
Member

  • It's not clear the best way to fix it - we could create a separate formula which is minimal and only has what we need, but I'm very hesitant to include internal-only formulae in homebrew-core.

My suggestion: move back to using gnu-tar. We're going to end up using it on Linux anyway, it supports the behaviour we need and it has no dependencies.

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

Testing will be needed to see if all bottle checksums remain consistent with that due to implementation differences between system bsdtar and gnu-tar, unless you suggest installing it everywhere.

gnu-tar probably makes heavy use of GNU extensions.

@MikeMcQuaid
Copy link
Member

@Bo98 As you said above:

I'm surprised it would affect anything like that given any tar writer can't really break compatibility with older/other tar readers.

There's --format=v7/--portability which (tested with ack) outputs the same checksum. Opened PR in Homebrew/brew#11251

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

I think that will exclude xattr support (need PAX headers for that).

bsdtar by default picks either --format=posix and --format=ustar. (Uses ustar if possible, unless it has xattrs etc that require the use of PAX).

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

What might be consistent is passing --format=posix to both bsdtar and gnu-tar.

@MikeMcQuaid
Copy link
Member

@Bo98 good shout (but went with --format=ustar as it's documented in both places).

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

ustar still doesn't support xattrs. posix is the same as pax so that might appear in the manual instead. libarchive definitely supports both names.

@MikeMcQuaid
Copy link
Member

ustar still doesn't support xattrs.

@Bo98 Yes. Why/when do we need them?

@MikeMcQuaid
Copy link
Member

libarchive definitely supports both names.

I'd rather not use a name that's not explicitly documented, even if it works.

Switching from the current libarchive / BSD tar default to always using pax breaks existing reproducibility i.e. the ack checksum changes. As a result, I'd like us to use ustar until it's demonstrated to be a problem.

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

Why/when do we need them?

I'm not entirely sure, I just know it's used in OpenJDK (albeit probably not too important).

Other restrictions we should be aware of though if we use ustar:

  • Cannot use filenames > 100 characters without GNU extensions
  • Cannot use base file paths (excluding file name) > 155 characters without GNU extensions
  • Cannot use non-ASCII paths (this is probably not a problem)
  • Cannot have files > 8GB
  • The mtime of the files must be <= Wed 16 Mar 2242 12:56:31 GMT, however some tar readers have a year 2038 restriction instead (though I don't think this affects bsdtar or gnu-tar).
  • Cannot use fflags (which I think is bsdtar specific anyway)
  • Can't use ACL stuff (we don't anyway)
  • Cannot use sparse files (which mean potentially bigger archives, but might actually dodge the sudo purge bug - but we should test whether this is handled properly and converted correctly to a non-sparse file).

Might be fine but worth noting the above anyway should we hit any.

ustar writing is fairly universal - this is the format that the Ruby TarWriter/minitar implementations write as. Although PAX has the advantages of avoiding the above, it does allow a bit more implementation variances to creep in, such as the fflags thing, because PAX allows easy addition of extensions (though this can be avoided with a couple flags), so I do agree that ustar is nice to use if none of the above is a problem.

the ack checksum changes

This is unsurprising since we are switching from a dynamic mode to a fixed format.

@MikeMcQuaid
Copy link
Member

Might be fine but worth noting the above anyway should we hit any.
This is unsurprising since we are switching from a dynamic mode to a fixed format.

Thanks for noting these 🎉

so I do agree that ustar is nice to use if none of the above is a problem.

Agreed. If/when we hit problems: we can add something to brew bottle to upgrade to pax if needed. What would be ideal is if gnu-tar had the same "use ustar unless you need pax and then use that" mode.

@Bo98
Copy link
Member

Bo98 commented Apr 26, 2021

  • Cannot use base file paths (excluding file name) > 155 characters without GNU extensions

Closest I got here was 146 characters in scons, so this is definitely something to watch.

Interestingly, the Ruby minitar supports the GNU extension here rather than going for full PAX support, so it seems to be used a bit outside gnu-tar (bsdtar very possibly supports reading it but not writing).

@MikeMcQuaid
Copy link
Member

Another option, just for completeness, would be using gnu-tar for all bottling everywhere.

@carlocab
Copy link
Member Author

Another option, just for completeness, would be using gnu-tar for all bottling everywhere.

Partial to this. It's simple, and I think is most likely to guarantee uniformity across all platforms/OS versions.

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@carlocab carlocab deleted the zstd-relocate branch May 9, 2021 22:16
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants