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

podman: includes missing edk2-aarch64-code.fd on aarch64-darwin #163015

Closed
wants to merge 1 commit into from
Closed

podman: includes missing edk2-aarch64-code.fd on aarch64-darwin #163015

wants to merge 1 commit into from

Conversation

tricktron
Copy link
Member

Motivation for this change

Podman on aarch64-darwin needs the edk2-aarch64-code.fd file to create the vm with qemu. This pr patches the getEdk2CodeFd function to find the edk2-aarch64-code.fd file under $out/share/qemu.

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.

@tricktron tricktron requested a review from zowoq March 6, 2022 14:25
@ofborg ofborg bot added the 6.topic: darwin Running or building packages on Darwin label Mar 6, 2022
@tricktron tricktron force-pushed the podman-fix-edk2-path branch from 966eb14 to 24d04d8 Compare March 6, 2022 14:50
zowoq
zowoq requested changes Mar 6, 2022
View reviewed changes
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.

No, we don't want to carry patches for this.

Lets get this fixed upsteam.

zowoq
zowoq reviewed Mar 6, 2022
View reviewed changes
pkgs/applications/virtualization/podman/default.nix
@@ -74,6 +83,9 @@ buildGoModule rec {
PREFIX=$out make install.completions
MANDIR=$man/share/man make install.man
runHook postInstall
'' + lib.optionalString (stdenv.isDarwin && stdenv.isAarch64) ''
mkdir $out/share/qemu
ln -s ${qemu}/share/qemu/edk2-aarch64-code.fd $out/share/qemu
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be done by adding it to the symlinkJoin in the wrapper anyway.

@zowoq
Copy link
Contributor

zowoq commented Mar 6, 2022

Not carrying patches has been discussed previously. Thanks anyway for the PR.

@zowoq zowoq closed this Mar 6, 2022
@tricktron
Copy link
Member Author

@zowoq Ok. So is there another solution/approach to this problem?

tricktron added a commit to tricktron/podman that referenced this pull request Mar 14, 2022
@tricktron
fix: removes edk2 directory entry to enable the patch
e482c85 
See NixOS/nixpkgs#163015.
@zowoq
Copy link
Contributor

zowoq commented Apr 17, 2022

Sorry, I responded to your comment on my issue in the upstream repo but I must have accidentally cleared this notification at the same time.

So is there another solution/approach to this problem?

That we want to have in the nixpkgs repo today, no.

A few points:

  • This isn't a nixpkgs specific issue, it is a "not homebrew" issue.
  • It has been broken for nixpkgs users since upstream first added podman machine a year ago and AFAIK I was the first person to tell upstream about it a few weeks ago.
  • podman machine works on linux as well so I assume this needs to be fixed in other places besides options_darwin_arm64 and the patch would also need to be applied on aarch64-linux. (applying it unconditionally would be better to avoid breakage, may actually be necessary if upstream supports aarch64 vm on x86_64?)
  • Adding qemu to the podman closure is exactly what we're trying to avoid by using the wrapper. We could patch it in via the wrapper but podman machine/qemu can get into a broken state anyway which why qemu isn't already included in the wrapper.
  • I very strongly believe that if we add this patch we'll end up carrying some variation of it indefinitely.
  • Carrying a patch invites more patches. Currently I have seven podman patches for darwin/linux that I could make a case for adding here, all of which could (and should) be fixed upstream.
  • podman is just one tool, buildah, cri-o, skopeo and others are all fast moving projects built on the same libraries and share the same config. Quite often a change to one should also be applied to the others to either fix the same problem or just to avoid causing different behaviour. (not relevant for this change at the moment but carrying a patch for one does add overhead for the other tools)

So while I appreciate that you're trying to fix what seems a relatively simple problem, long term maintainability is a significant concern.

@zowoq zowoq mentioned this pull request Apr 17, 2022
Open
@zowoq zowoq mentioned this pull request Dec 6, 2022
Closed
This was referenced Jan 7, 2023
Closed
Merged
@n8henrie n8henrie mentioned this pull request Jan 22, 2023
Closed
n8henrie added a commit to n8henrie/podman that referenced this pull request Feb 7, 2023
@n8henrie
Resolve symlink path for qemu directory if possible
6b64589 
Fixes containers#17026
Fixes NixOS/nixpkgs#169118

[NO NEW TESTS NEEDED]

Related: NixOS/nixpkgs#163015
Signed-off-by: Nathan Henrie <[email protected]>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/podman that referenced this pull request Feb 8, 2023
@n8henrie
Resolve symlink path for qemu directory if possible
a1cc373 
Fixes containers#17026
Fixes NixOS/nixpkgs#169118

[NO NEW TESTS NEEDED]

Related: NixOS/nixpkgs#163015
Signed-off-by: Nathan Henrie <[email protected]>
@n8henrie n8henrie mentioned this pull request Apr 24, 2023
Closed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/out-share-linked-with-nix-profile-install-but-not-otherwise/27561/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zowoq zowoq zowoq requested changes

@vdemeester vdemeester Awaiting requested review from vdemeester

@marsam marsam Awaiting requested review from marsam

@kalbasit kalbasit Awaiting requested review from kalbasit

@adisbladis adisbladis Awaiting requested review from adisbladis

@saschagrunert saschagrunert Awaiting requested review from saschagrunert

Assignees
No one assigned
Labels
6.topic: darwin Running or building packages on Darwin 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tricktron @zowoq @nixos-discourse