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.

Sign up for GitHub

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

Jump to bottom

dev-cmd/bottle: set uid/gid, use libarchive on macOS. #11167

Merged
merged 1 commit into from Apr 21, 2021
Merged

dev-cmd/bottle: set uid/gid, use libarchive on macOS. #11167

merged 1 commit into from Apr 21, 2021

Conversation

MikeMcQuaid
Copy link
Member

@MikeMcQuaid MikeMcQuaid commented Apr 16, 2021 •
edited

Take 2 on #11165 but use newish libarchive consistently on macOS.

@MikeMcQuaid MikeMcQuaid added the critical Critical change which should be shipped as soon as possible. label Apr 16, 2021
@BrewTestBot
Copy link
Member

BrewTestBot commented Apr 16, 2021 •
edited

Review period ended.

BrewTestBot
BrewTestBot previously approved these changes Apr 16, 2021
View reviewed changes
@MikeMcQuaid MikeMcQuaid mentioned this pull request Apr 16, 2021
Closed
@Bo98
Copy link
Member

Bo98 commented Apr 16, 2021

Installing libarchive is also an alternative - it might make it more consistent with other OS versions? There could be slight implementation differences between BSD and GNU tar.

Bonus: it's keg only so less likely to affect other builds.

@carlocab
Copy link
Member

I'd err on using libarchive everywhere, but perhaps I am overly paranoid about the changes between different versions.

@MikeMcQuaid
Copy link
Member Author

Installing libarchive is also an alternative - it might make it more consistent with other OS versions? There could be slight implementation differences between BSD and GNU tar.

Bonus: it's keg only so less likely to affect other builds.

Nice idea, I like this 👍🏻

@MikeMcQuaid MikeMcQuaid changed the title dev-cmd/bottle: set uid/gid, use gnu-tar. dev-cmd/bottle: set uid/gid, use libarchive. Apr 20, 2021
@MikeMcQuaid MikeMcQuaid changed the title dev-cmd/bottle: set uid/gid, use libarchive. dev-cmd/bottle: set uid/gid, use libarchive on macOS. Apr 20, 2021
@MikeMcQuaid
Copy link
Member Author

Installing libarchive is also an alternative - it might make it more consistent with other OS versions? There could be slight implementation differences between BSD and GNU tar.

I'm going for this but just on macOS. I want to avoid causing grief for Linux folks for now but we can consider making it default/required on Linux in future.

@MikeMcQuaid MikeMcQuaid force-pushed the bottle-uid-gid-gtar branch 3 times, most recently from 4fb92c0 to 98dfeab Compare April 20, 2021 13:27
@MikeMcQuaid
Copy link
Member Author

@thomasrockhu codecov/codecov-bash@1b4b96a seems to have broken our CI. Help?

@thomasrockhu thomasrockhu mentioned this pull request Apr 20, 2021
Merged
7 tasks
@thomasrockhu
Copy link
Contributor

thomasrockhu commented Apr 20, 2021 •
edited

@MikeMcQuaid I opened a PR that should fix the issue

@MikeMcQuaid MikeMcQuaid dismissed a stale review via 03efccb April 21, 2021 08:02
@MikeMcQuaid
dev-cmd/bottle: set uid/gid, use libarchive on macOS.
4a3fc2a 
Take 2 on #11165 but use newish `libarchive` consistently on macOS.
@MikeMcQuaid MikeMcQuaid added waiting for feedback Merging is blocked until sufficient time has passed for review and removed critical Critical change which should be shipped as soon as possible. labels Apr 21, 2021
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Apr 21, 2021
carlocab
carlocab approved these changes Apr 21, 2021
View reviewed changes
Library/Homebrew/dev-cmd/bottle.rb
bsdtar_args = ["--uid", "0", "--gid", "0"].freeze

# System bsdtar is new enough on macOS Catalina and above.
return bsdtar_args if OS.mac? && MacOS.version >= :catalina
Copy link
Member

Choose a reason for hiding this comment

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

You can drop the OS.mac? check here. MacOS.version >= :anything always evaluates to false on Linux. (And MacOS.version < :anything is always true.) But keeping it too is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

@carlocab Nope 😁. On brew tests --generic both OS.mac? and OS.linux? are false (and MacOS isn't defined). This layer exists to make it easier to potentially port to other OSs in future.

Copy link
Member

Choose a reason for hiding this comment

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

Ah right. Generic OS. So often forgotten 😢

@MikeMcQuaid MikeMcQuaid merged commit 3156b53 into Homebrew:master Apr 21, 2021
@MikeMcQuaid MikeMcQuaid deleted the bottle-uid-gid-gtar branch April 21, 2021 09:44
@github-actions github-actions bot added the outdated PR was locked due to age label May 22, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carlocab carlocab carlocab approved these changes

@BrewTestBot BrewTestBot Awaiting requested review from BrewTestBot

Assignees
No one assigned
Labels
outdated PR was locked due to age
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@MikeMcQuaid @BrewTestBot @Bo98 @carlocab @thomasrockhu