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

containerd: fix --version output #85269

Merged
merged 1 commit into from May 6, 2020
Merged

Conversation

@euank
Copy link
Contributor

@euank euank commented Apr 15, 2020

Before this change, 'containerd --version' with the nix package wouldn't
print useful version information.

In addition, the build output a bunch of (harmless) errors about 'git:
command not found'.

This fixes those problems.

cc @vdemeester

Here's the difference from this change:

# Before
$ nix-build -E 'with import <nixpkgs> {}; callPackage ./default.nix {}'
...
building
patching script interpreter paths in .
...
make: git: Command not found
... (repeated several times)

$ ./result-bin/bin/containerd --version
containerd github.com/containerd/containerd  .m

# After
# No git errors on building it, and...
$ ./result-bin/bin/containerd --version
containerd github.com/containerd/containerd v1.2.13 7ad184331fa3e55e52b890ea95e65ba581ae3429
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Member

@vdemeester vdemeester left a comment

So, I know this is a kinda weird hack. I did some grepping around, and couldn't find any other package currently in nixpkgs that deals with this issue.
I have no clue if this is a reasonable hack to include over here or what, and I'm happy to abandon this change if it seems a little too hacky.

Nice hack 😝 I am not sure either if it's a reasonable hack or not, but at least it works 👍

@euank euank force-pushed the euank:containerd-versioning branch 2 times, most recently from 47b04ef to 9d579af Apr 15, 2020
@euank
Copy link
Contributor Author

@euank euank commented Apr 15, 2020

Oh, it turns out I was blind; for some reason I mis-remembered that overriding some makefile variables would prevent the assignment from being evaluating, but others it wouldn't.

It turns out that's wrong, and I can just set make REVISION=.... to avoid it calling git entirely.

Well, that makes this PR much simpler. One second ...

@euank euank force-pushed the euank:containerd-versioning branch from 9d579af to ce132cb Apr 15, 2020
@ofborg ofborg bot requested a review from vdemeester Apr 15, 2020
@euank
Copy link
Contributor Author

@euank euank commented Apr 15, 2020

Okay, PR updated and I verified the version flag indeed works as I'd expect still.

It's now a much simpler and less weird change. I think this should be quite straightforward to merge :)

Copy link
Member

@vdemeester vdemeester left a comment

LGTM 🐯

Before this change, 'containerd --version' with the nix package wouldn't
print useful version information.

In addition, the build output a bunch of (harmless) errors about 'git:
command not found'.

This fixes both of those problems.
@euank euank force-pushed the euank:containerd-versioning branch from ce132cb to 6d3eaa0 Apr 15, 2020
@ofborg ofborg bot requested a review from vdemeester Apr 15, 2020
@vdemeester
Copy link
Member

@vdemeester vdemeester commented Apr 15, 2020

@GrahamcOfBorg build containerd

@vdemeester
Copy link
Member

@vdemeester vdemeester commented May 6, 2020

@offlinehacker offlinehacker merged commit 85ee3e1 into NixOS:master May 6, 2020
18 checks passed
18 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
containerd on aarch64-linux Success
Details
containerd on x86_64-linux Success
Details
containerd, containerd.passthru.tests on aarch64-linux Success
Details
containerd, containerd.passthru.tests on x86_64-linux Success
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="6d3eaa0"; rev="6d3eaa0527fc02c486889364afe148809834f40f"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="6d3eaa0"; rev="6d3eaa0527fc02c486889364afe148809834f40f"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="6d3eaa0"; rev="6d3eaa0527fc02c486889364afe148809834f40f"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="6d3eaa0"; rev="6d3eaa0527fc02c486889364afe148809834f40f"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="6d3eaa0"; rev="6d3eaa0527fc02c486889364afe148809834f40f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="6d3eaa0"; rev="6d3eaa0527fc02c486889364afe148809834f40f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="6d3eaa0"; rev="6d3eaa0527fc02c486889364afe148809834f40f"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.