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

Jump to bottom

cri-o, buildah: add wrappers #86637

Merged
merged 5 commits into from May 25, 2020
Merged

cri-o, buildah: add wrappers #86637

merged 5 commits into from May 25, 2020

Conversation

zowoq
Copy link
Contributor

@zowoq zowoq commented May 3, 2020 •
edited

Opened as a draft, waiting for other changes to land in master.

Having a wrapper might be preferred but for now I'd rather that we merge this and reevaluate if we end up adding more packages.

Ended up being easier to add the wrapper.

cc @NixOS/podman

@zowoq zowoq marked this pull request as draft May 3, 2020 08:49
@zowoq zowoq changed the title cri-o: wrap packages nixos/cri-o: set paths May 5, 2020
@zowoq zowoq marked this pull request as ready for review May 5, 2020 03:09
saschagrunert
saschagrunert approved these changes May 5, 2020
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@zowoq zowoq marked this pull request as draft May 5, 2020 09:41
@zowoq zowoq mentioned this pull request May 5, 2020
Merged
10 tasks
@zowoq zowoq changed the title nixos/cri-o: set paths cri-o, buildah: add wrappers May 12, 2020
@zowoq zowoq removed the request for review from nlewo May 17, 2020 02:26
@zowoq zowoq marked this pull request as ready for review May 17, 2020 02:29
@zowoq
Copy link
Contributor Author

zowoq commented May 17, 2020 •
edited

Needs to be rebased after #86290 is merged.

Done.

zowoq
zowoq commented May 17, 2020
View reviewed changes
nixos/modules/virtualisation/cri-o.nix

[crio.runtime]
conmon = "${pkgs.conmon}/bin/conmon"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@saschagrunert Apologies for asking you to keep this in #86290 and then removing it here anyway, the wrapper end up being the best approach.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah no worries ❤️

@ofborg ofborg bot requested a review from kalbasit May 17, 2020 02:37
@Profpatsch
Copy link
Member

I merged #86290 so this should be good to go?

@zowoq
Copy link
Contributor Author

zowoq commented May 25, 2020

I merged #86290 so this should be good to go?

Yes, thanks @Profpatsch

Profpatsch
Profpatsch reviewed May 25, 2020
View reviewed changes
pkgs/development/tools/buildah/wrapper.nix
mkdir -p $out/bin
ln -s ${buildah-unwrapped}/share $out/share
makeWrapper ${buildah-unwrapped}/bin/buildah $out/bin/buildah \
--prefix PATH : ${binPath}
Copy link
Member

Choose a reason for hiding this comment

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

iirc you don’t need a separate derivation to wrap the executable, you can just run makeWrapper in the postInstall phase of the original derivation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This and the top-level -unwrapped are duplicated from podman.

For now I'd like to keep them as duplicates so they are easier to merge into one wrapper. I'm looking at doing that after adding some tests.

pkgs/top-level/all-packages.nix
@@ -1369,7 +1369,8 @@ in

btfs = callPackage ../os-specific/linux/btfs { };

buildah = callPackage ../development/tools/buildah { };
buildah = callPackage ../development/tools/buildah/wrapper.nix { };
buildah-unwrapped = callPackage ../development/tools/buildah { };
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to expose the unwrapped version?

saschagrunert
saschagrunert approved these changes May 25, 2020
View reviewed changes
pkgs/applications/virtualization/cri-o/wrapper.nix
, utillinux # nsenter
, cni-plugins # not added to path
, iptables
}:
Copy link
Member

Choose a reason for hiding this comment

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

We still need socatfor port forwarding purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@ofborg ofborg bot requested review from saschagrunert and Profpatsch May 25, 2020 07:22
saschagrunert
saschagrunert approved these changes May 25, 2020
View reviewed changes
Copy link
Member

@saschagrunert saschagrunert left a comment

Choose a reason for hiding this comment

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

LGTM

@Profpatsch Profpatsch merged commit 37a87c3 into NixOS:master May 25, 2020
@zowoq zowoq deleted the crio-wrapper branch May 25, 2020 10:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes

@vdemeester vdemeester Awaiting requested review from vdemeester

@adisbladis adisbladis Awaiting requested review from adisbladis

@kalbasit kalbasit Awaiting requested review from kalbasit

@Profpatsch Profpatsch Awaiting requested review from Profpatsch

Assignees
No one assigned
Labels
6.topic: nixos 8.has: clean-up 8.has: module (update) 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@zowoq @Profpatsch @saschagrunert