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

conftest: fix version command #99557

Merged
merged 2 commits into from Nov 19, 2020
Merged

conftest: fix version command #99557

merged 2 commits into from Nov 19, 2020

Conversation

@06kellyjac
Copy link
Contributor

@06kellyjac 06kellyjac commented Oct 4, 2020

Motivation for this change

Fix conftest --version so it outputs the version

Things done

Now outputs the following:

$ conftest --version
Version: 0.21.0
Commit:
Date:

Also added a longDescription

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS (x86_64)
    • 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.
@06kellyjac 06kellyjac changed the title Conftest version conftest: fix version command Oct 4, 2020
@ofborg ofborg bot requested review from yurrriq and kalbasit Oct 4, 2020
@yurrriq
yurrriq approved these changes Oct 6, 2020
Copy link
Member

@yurrriq yurrriq left a comment

lgtm, thanks!

λΠ nix-shell -I nixpkgs=https://github.com/NixOS/nixpkgs/tarball/pull/99557/head -p conftest --run 'conftest --version'
unpacking 'https://github.com/NixOS/nixpkgs/tarball/pull/99557/head'...
these derivations will be built:
  /nix/store/n7qzhx2yv1r81578p0q318whaajnscbf-remove-references-to.drv
  /nix/store/c7qjsn8ibg62bwcngxxk3ynbxqkd3fla-conftest-0.21.0.drv
these paths will be fetched (94.48 MiB download, 418.09 MiB unpacked):
  /nix/store/18ggindri5xm8zbdpzy7x2927z1lg5gc-stdenv-linux
  /nix/store/3qhafmf9yj5nyyrnm9xnil6vl6gglsf5-bash-interactive-4.4-p23-info
  /nix/store/546gk8pnvdya2m2a6y791ppr71wj2imq-bash-interactive-4.4-p23-doc
  /nix/store/fabkq7vbl5x6q85rg150sxr1g3rixyys-go-1.15.2
  /nix/store/g5asxygf03fbkbr9h8sw15ac978bjppg-conftest-0.21.0-go-modules
  /nix/store/l2g6my7nykfshahijqnk7gy7s5lq2r3k-bash-interactive-4.4-p23-man
  /nix/store/ly74nfilzwx6raicvc53jdplhh4f6b0r-source
...
Version: 0.21.0
Commit: 
Date: 
@nixos-discourse
Copy link

@nixos-discourse nixos-discourse commented Oct 22, 2020

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/346

@06kellyjac
Copy link
Contributor Author

@06kellyjac 06kellyjac commented Nov 17, 2020

/marvin opt-in
@yurrriq thanks for the review, could you post a comment with /status needs_merger? The bot needs someone who isn't the PR author to do it

@marvin-mk2 marvin-mk2 bot added the marvin label Nov 17, 2020
@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 17, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@marvin-mk2
Copy link

@marvin-mk2 marvin-mk2 bot commented Nov 17, 2020

The PR author cannot set the status to needs_merger. Please wait for an external review.

If you are not the PR author and you are reading this, please review the usage of this bot. You may be able to help. Please make an honest attempt to resolve all outstanding issues before setting to needs_merger.

@06kellyjac
Copy link
Contributor Author

@06kellyjac 06kellyjac commented Nov 17, 2020

Result of nixpkgs-review pr 99557 1

1 package built:
  • conftest
@yurrriq
Copy link
Member

@yurrriq yurrriq commented Nov 19, 2020

/status needs_merger

@marvin-mk2 marvin-mk2 bot added the needs_merger label Nov 19, 2020
@marvin-mk2 marvin-mk2 bot requested a review from kevincox Nov 19, 2020
@marvin-mk2 marvin-mk2 bot added awaiting_merger and removed needs_merger labels Nov 19, 2020
"-ldflags="
"-s"
"-w"
"-X github.com/open-policy-agent/conftest/internal/commands.version=${version}"

This comment has been minimized.

@kevincox

kevincox Nov 19, 2020
Contributor

Is it possible to document these? Using a long form would be ideal but if not just giving a couple of words each helps readers looking for something quickly.

This comment has been minimized.

@06kellyjac

06kellyjac Nov 19, 2020
Author Contributor

I'm not sure it's worth documenting buildFlagsArray/ldflags as most people packaging know what they are and they're easy to look up in ld documentation. As the variable name says they're flags for ld.
Also there are many many many packages with almost identical inputs that aren't documented.

I wouldn't add a comment above vendorSha256 explaining what it's for or add a comment for why there's rec in buildGoModule rec {

I'm happy to remove -s and -w though as now that I think about it they're not particularly necessary, it was mostly leftover from another derivation

This comment has been minimized.

@kevincox

kevincox Nov 19, 2020
Contributor

Yeah. Removing -s and -w makes sense, and removes two of the things to question. I honestly can't find out what -X is. I agree, -ldflags is obvious enough.

@kevincox
Copy link
Contributor

@kevincox kevincox commented Nov 19, 2020

I'm going to megre for now. But feel free to send me a follow up with some comments if you think that is a good idea.

@kevincox kevincox merged commit b50e2ec into NixOS:master Nov 19, 2020
19 checks passed
19 checks passed
tests tests
Details
action
Details
conftest, conftest.passthru.tests on aarch64-linux No attempt
Details
Evaluation Performance Report Evaluator Performance Report
Details
Wait for ofborg
Details
conftest, conftest.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="94fd565"; rev="94fd56594a6543fac73e972952d87ed635cfbda4"; } ./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="94fd565"; rev="94fd56594a6543fac73e972952d87ed635cfbda4"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94fd565"; rev="94fd56594a6543fac73e972952d87ed635cfbda4"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94fd565"; rev="94fd56594a6543fac73e972952d87ed635cfbda4"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94fd565"; rev="94fd56594a6543fac73e972952d87ed635cfbda4"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94fd565"; rev="94fd56594a6543fac73e972952d87ed635cfbda4"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="94fd565"; rev="94fd56594a6543fac73e972952d87ed635cfbda4"; } ./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
@06kellyjac 06kellyjac deleted the 06kellyjac:conftest_version branch Nov 19, 2020
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

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