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

podman: make work on darwin #157134

Closed
wants to merge 2 commits into from
Closed

Conversation

eraserhd
Copy link
Contributor

  • podman: Use path to find gvproxy on Darwin
  • Revert "podman: remove darwin wrapper"
Motivation for this change

Make podman work on Mac.

This is a temporary solution to #141041, with notes on how to work with upstream's change once 4.0.0 is released.

This restores the wrapper for darwin. @zowoq mentions that this wrapper somehow wrote to the store, but I don't
see how that could be unless there is a problem with one of packages passed to lib.makeBinPath (in which case
removing the wrapper isn't the right fix).

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@marsam @zowoq

Closes NixOS#141041

Once 4.0.0 is released, there will be an environment variabled
that we can set in the wrapper.  Until then, this allows
`podman machine start` to work on Darwin.
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Jan 28, 2022
Copy link
Contributor

@zowoq zowoq left a comment

Choose a reason for hiding this comment

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

This doesn't fix the problem I mentioned in #141041 (comment).

this wrapper somehow wrote to the store, but I don't see how that could be unless there is a problem

What I said was:

store paths get written to the vm config

See: #141041 (comment)

@zowoq
Copy link
Contributor

zowoq commented Jan 28, 2022

Also the podman maintainers have previously discussed this sort of patching and we really don't want to carry what will basically be a permanent patch.

It may not seem like much to add a patch but needing to keep the podman, etc ecosystem in sync and up to date makes permanent patching problematic.

podman used find gvproxy from $PATH until upstream changed it to what was basically the worst possible option for us, it would really be for the best that it get resolved upstream. Disregard, got mixed up between gvproxy and qemu.

@zowoq zowoq closed this Jan 28, 2022
@eraserhd
Copy link
Contributor Author

Hold up. I'm very confused.

So the package is broken and doesn't work on darwin, so we are removing the wrapper -- which AFAICT is unrelated to the breakage, but breaks it further. But we neither mark the package as broken nor remove darwin from its supported platforms?

And this solution contains a patch literally labelled "temporary", with a reference to the already-merged upstream fix, and instructions for when the already-merged upstream fix is released, but you are closing it because we won't carry permanent patches?

I can understand many reasons for not wanting to merge this, but your stated reasons don't make sense. Did you read the code?

@eraserhd eraserhd reopened this Jan 28, 2022
Copy link
Contributor

@zowoq zowoq left a comment

Choose a reason for hiding this comment

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

Sorry, seems like I managed to drop a comment from a review.

By permanent patching I was referring to patching to fix the qemu paths we could re-add the wrapper.

# path. When 4.0.0 is released, this goes away and the wrapper should set
# $CONTAINERS_HELPER_BINARY_DIR instead:
# https://github.com/containers/podman/blob/main/vendor/github.com/containers/common/pkg/config/config.go#L1171
./0001-Look-for-gvproxy-in-PATH.patch
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't want to add this patch with 4.0.0 being released soon as it will complicate the update, we can deal with darwin after we've completed the upgrade.

@zowoq zowoq closed this Jan 28, 2022
@zowoq
Copy link
Contributor

zowoq commented Jan 28, 2022

But we neither mark the package as broken nor remove darwin from its supported platforms?

podman can still be used as a remote, it's only podman machine (which by having the wrapper implies that we support it) that is broken.

@ofborg ofborg bot requested a review from zowoq January 28, 2022 13:32
@eraserhd
Copy link
Contributor Author

Aha, I get it. Sorry for the grumpy.

@zowoq
Copy link
Contributor

zowoq commented Feb 25, 2022

4.0 update is merged, PR to wrap gvproxy: #161130

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants