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

containerd: fix --version output #85269

Merged
merged 1 commit into from
May 6, 2020
Merged

containerd: fix --version output #85269

merged 1 commit into from
May 6, 2020
+4 −2

Conversation

euank
Copy link
Member

@euank euank commented Apr 15, 2020 •
edited
Loading

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.

Sorry, something went wrong.

@ofborg ofborg bot requested review from vdemeester and offlinehacker April 15, 2020 05:47
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 15, 2020
vdemeester
vdemeester reviewed Apr 15, 2020
View reviewed changes
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.

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 containerd-versioning branch 2 times, most recently from 47b04ef to 9d579af Compare April 15, 2020 06:17
@euank
Copy link
Member Author

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 containerd-versioning branch from 9d579af to ce132cb Compare April 15, 2020 06:23
@ofborg ofborg bot requested a review from vdemeester April 15, 2020 06:24
@euank
Copy link
Member Author

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 :)

vdemeester
vdemeester approved these changes Apr 15, 2020
View reviewed changes
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 🐯

@euank
containerd: fix --version output

Unverified

The email in this signature doesn’t match the committer email.
GPG key ID: B2D065AD4D6E0C81
Expired
Learn about vigilant mode
6d3eaa0 
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 containerd-versioning branch from ce132cb to 6d3eaa0 Compare April 15, 2020 06:28
@ofborg ofborg bot requested a review from vdemeester April 15, 2020 06:29
@vdemeester
Copy link
Member

@GrahamcOfBorg build containerd

@vdemeester
Copy link
Member

ping @offline @nlewo @Mic92

@offlinehacker offlinehacker merged commit 85ee3e1 into NixOS:master May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@offlinehacker offlinehacker

@vdemeester vdemeester

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@euank @vdemeester @offlinehacker