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

docker_24: init at 24.0.5 #225309

Merged
merged 5 commits into from
Aug 1, 2023
Merged

docker_24: init at 24.0.5 #225309

merged 5 commits into from
Aug 1, 2023

Conversation

mikroskeem
Copy link
Member

@mikroskeem mikroskeem commented Apr 8, 2023

Description of changes

#223362

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/)
  • 23.05 Release Notes (or backporting 22.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
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Apr 8, 2023
@amaxine
Copy link
Contributor

amaxine commented Apr 8, 2023

Thanks for looking into this. When I reviewed changes when 23 was first getting RCs, the primary thing that'll need fixing in the build process is documentation - everything else should apply to 20.x and 23.x.

@noisersup
Copy link
Member

What's the state of this one and if there's anything that can help to kick this one forward?

@vito
Copy link

vito commented Jun 1, 2023

Thanks for working on this! Any chance we could jump ahead to v24.0.2 at this point? 😇 It has some features in its upgraded Buildkit dependency which I'd like to use (chiefly oci-layout:// support).

@mikroskeem mikroskeem changed the title docker_23_0: init at 23.0.3 docker_24_0: init at 24.0.2 Jun 1, 2023
@mikroskeem mikroskeem force-pushed the update/docker-23-x branch from 49c5063 to d5dba53 Compare June 1, 2023 15:52
@mikroskeem
Copy link
Member Author

What's the state of this one and if there's anything that can help to kick this one forward?

It's been stuck behind my time schedule and https://gist.github.com/mikroskeem/14c8f08ac8ad14e3f4f889f9d5a2b427

@mikroskeem mikroskeem force-pushed the update/docker-23-x branch from d5dba53 to 5c9df7b Compare June 1, 2023 16:35
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 1, 2023
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jun 1, 2023
@vito
Copy link

vito commented Jun 2, 2023

I'm able to get this building with two small changes:

diff --git a/pkgs/applications/virtualization/docker/default.nix b/pkgs/applications/virtualization/docker/default.nix
index 5f76710e21a..51b74bbdf12 100644
--- a/pkgs/applications/virtualization/docker/default.nix
+++ b/pkgs/applications/virtualization/docker/default.nix
@@ -176,7 +176,7 @@ rec {
     nativeBuildInputs = [
       makeWrapper pkg-config go-md2man go libtool installShellFiles
     ];
-    buildInputs = plugins;
+    buildInputs = [ glibc glibc.static ] ++ plugins;
 
     postPatch = ''
       patchShebangs man scripts/build/
@@ -280,7 +280,7 @@ rec {
     cliRev = "v${version}";
     cliHash = "sha256-Z8bj+TsQVhoQav7+0MQz9Iy4VJCuSX/Cm+sWytev2ns=";
     mobyRev = "v${version}";
-    mobyHash = "sha256-Z8bj+TsQVhoQav7+0MQz9Iy4VJCuSX/Cm+sWytev2ns=";
+    mobyHash = "sha256-AKS1x0zo+FRpx2HXyM7I+PiwEZj/jCWebd5LIjLw9zc=";
     runcRev = "v1.1.7";
     runcHash = "sha256-reSC9j9ESjRigItBRytef78XBjmMGsqu0o9qcN2AstU=";
     containerdRev = "v1.7.1";

Guessing the duped hash was just a typo, took a bit to figure out!

Without adding glibc, building fails like so - guessing this was only working due to some kind of impurity before:

Logs
error: builder for '/nix/store/sif28jpm25b1n6r2c8ynli6ngxlaxxk8-docker-24.0.2.drv' failed with exit code 1;
       last 10 log lines:
       > configuring
       > building
       > # github.com/docker/cli/cmd/docker
       > /nix/store/181ggl7y9szajk7hbj3drimkxw63ix1h-go-1.20.4/share/go/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
       > /nix/store/shw0b6wv2xdvyj71b1fj147i83awrqfz-binutils-2.40/bin/ld: cannot find -lresolv: No such file or directory
       > /nix/store/shw0b6wv2xdvyj71b1fj147i83awrqfz-binutils-2.40/bin/ld: cannot find -lpthread: No such file or directory
       > /nix/store/shw0b6wv2xdvyj71b1fj147i83awrqfz-binutils-2.40/bin/ld: cannot find -ldl: No such file or directory
       > /nix/store/shw0b6wv2xdvyj71b1fj147i83awrqfz-binutils-2.40/bin/ld: cannot find -lc: No such file or directory
       > collect2: error: ld returned 1 exit status
       >
       For full logs, run 'nix log /nix/store/sif28jpm25b1n6r2c8ynli6ngxlaxxk8-docker-24.0.2.drv'.

I spent some time trying to get the man pages to build, but it's a bit of a pain. The man page generation code is in a separate ./man/ Go module with an empty go.mod that doesn't declare its dependencies. Instead the upstream repo expects you to run ./scripts/docs/generate-man.sh, which depends on the network.

I see a few options:

  1. Just don't build man pages, which is a regression, but considering you can just use --help this seems to be the lesser evil to being stuck on Docker 20.
  2. Statically commit generated man pages in this repo. Right now they're 17,424 lines of text in total. We'd have to make sure to re-generate these on every bump, and include man pages for each supported version.
  3. Commit generated man pages to a separate, tagged repo. No more vendoring (yay!) but now there's another repo someone has to maintain (boo!).
  4. Change things upstream.

Personally, my preference is 1 but I don't know if lack of man pages is a dealbreaker. When 4 happens we can always bring them back, and until then having them gone is pressure for someone (maybe even me) to fix it someday.

What do y'all think?

@Philipp-M
Copy link
Member

I generally think going incremental is better than having a working PR open forever (don't let perfect be the enemy of good), so I would rather go with 1. I don't think it's really a deal breaker (since --help is always an option, and otherwise google).

We can create a follow-up PR to fix this (probably an issue before) or patch it upstream.

@mikroskeem mikroskeem changed the title docker_24_0: init at 24.0.2 docker_24_0: init at 24.0.4 Jul 9, 2023
@mikroskeem mikroskeem force-pushed the update/docker-23-x branch from 5c9df7b to c4fcd44 Compare July 9, 2023 15:06
@mikroskeem mikroskeem marked this pull request as ready for review July 9, 2023 16:42
@mikroskeem
Copy link
Member Author

I've opted in for 1. choice - let's get this update out.

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

@vito on 4. is there an upstream issue about this ? If I understood correctly (maybe not 😛), only the man generation part is a problem and require use to add glibc right ?

pkgs/applications/virtualization/docker/default.nix Outdated Show resolved Hide resolved
@mikroskeem
Copy link
Member Author

only the man generation part is a problem and require use to add glibc right ?

No. If you try to remove it, you'll see linker complaining about missing -lc -lresolv etc.

@mikroskeem mikroskeem force-pushed the update/docker-23-x branch from 31fa421 to 3c6cf77 Compare July 13, 2023 09:41
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@ofborg ofborg bot requested a review from vdemeester July 13, 2023 10:20
@marcofeltmann
Copy link

I ran into this issue docker/cli#4437 on unstable due to Go 1.20.6.
Since I require both for daily business (Go Services, Kubernetes, KinD) I'd really appreciate an upgrade.

@mikroskeem
Copy link
Member Author

Waiting for someone with permissions to merge, or do additional review.

Copy link
Contributor

@amaxine amaxine left a comment

Choose a reason for hiding this comment

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

Do you mind rebasing on latest master and bumping to 24.0.5? That contains fixes for go 1.20.6. I'll take a proper look after that.

@mikroskeem mikroskeem force-pushed the update/docker-23-x branch from 3c6cf77 to d60504f Compare July 27, 2023 09:06
mikroskeem added a commit to mikroskeem/nixpkgs that referenced this pull request Jul 27, 2023
@mikroskeem mikroskeem force-pushed the update/docker-23-x branch from d60504f to b841b17 Compare July 27, 2023 09:18
@ofborg ofborg bot requested a review from amaxine July 27, 2023 09:41
@teutat3s
Copy link
Member

Result of nixpkgs-review pr 225309 run on x86_64-linux 1

21 packages built:
  • apptainer
  • apptainer-overriden-nixos
  • apx
  • charliecloud
  • docker (docker_20_10)
  • docker-client
  • docker-client.man
  • docker-gc
  • docker.man (docker_20_10.man)
  • docker_24_0
  • fn-cli
  • nvidia-docker
  • pipework
  • python310Packages.jupyter-repo2docker
  • python310Packages.jupyter-repo2docker.dist
  • python311Packages.jupyter-repo2docker
  • python311Packages.jupyter-repo2docker.dist
  • singularity
  • singularity-overriden-nixos
  • udocker
  • udocker.dist

@amaxine amaxine changed the title docker_24_0: init at 24.0.4 docker_24_0: init at 24.0.5 Aug 1, 2023
Copy link
Contributor

@amaxine amaxine left a comment

Choose a reason for hiding this comment

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

Thank you, looks good, we can hopefully get man pages working again later. i think we probably want to call this docker_24 rather than docker_24_0 because semver. It's unlikely we'd want 24.0 and 24.1 packaged together.

@amaxine
Copy link
Contributor

amaxine commented Aug 1, 2023

Thanks a bunch for doing the work on this, it's really appreciated @mikroskeem

@amaxine amaxine merged commit 9313322 into NixOS:master Aug 1, 2023
@ofborg ofborg bot requested a review from amaxine August 1, 2023 21:09
@mikroskeem mikroskeem deleted the update/docker-23-x branch August 1, 2023 21:33
@mikroskeem mikroskeem changed the title docker_24_0: init at 24.0.5 docker_24: init at 24.0.5 Aug 1, 2023

patches = [

buildInputs = plugins ++ lib.optionals (lib.versionAtLeast version "23") [
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work on aarch64-darwin:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 11-100 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants