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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

cosign: use buildFlagsArray #120034

Merged
merged 1 commit into from Apr 21, 2021
Merged

cosign: use buildFlagsArray #120034

merged 1 commit into from Apr 21, 2021

Conversation

06kellyjac
Copy link
Member

@06kellyjac 06kellyjac commented Apr 21, 2021

Motivation for this change

Bump cosign to 0.3.1 Bump already done, details below still apply to what was changed in #120029

Add ldflags to make the release smaller (21 MB -> 19 MB) and version info

The upstream release looks like follows:

GitVersion:    v0.3.1
GitCommit:     76fde761a4e016c57293113ee1362f1c24a647cb
GitTreeState:  clean
BuildDate:     '2021-04-20T22:40:17Z'
GoVersion:     go1.16.3
Compiler:      gc
Platform:      linux/amd64

Left out the other parts that are commonly skipped in nixpkgs like date etc as they're better left as the default

GitVersion:    v0.3.1
GitCommit:     unknown
GitTreeState:  unknown
BuildDate:     unknown
GoVersion:     go1.16.3
Compiler:      gc
Platform:      linux/amd64

Move the ldflags change to set buildArrayFlags directly within bash. I was going to link the linter that catches this & the reason behind this change but I can't find it now 馃檭 @SuperSandro2000 do you remember?

I've also added myself as a maintainer if you dont mind @LeSuisse

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.

@LeSuisse
Copy link
Contributor

Done in #120029

I've also added myself as a maintainer if you dont mind @LeSuisse

Not at all 馃憤

@06kellyjac
Copy link
Member Author

Ah sorry, I had a look for the PR but I missed it 馃槄
I'll rebase now

And add myself as a maintainer
@@ -21,13 +21,15 @@ buildGoModule rec {

subPackages = [ "cmd/cosign" ];

buildFlagsArray = [ "-ldflags=-s -w -X github.com/sigstore/cosign/cmd/cosign/cli.gitVersion=${version}" ];
preBuild = ''
buildFlagsArray+=("-ldflags" "-s -w -X github.com/sigstore/cosign/cmd/cosign/cli.gitVersion=v${version}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a difference with buildFlagsArray?

It seems the documentation recommends its usage:

### `buildFlagsArray` and `buildFlags`: {#ex-goBuildFlags-noarray}
These attributes set build flags supported by `go build`. We recommend using `buildFlagsArray`. The most common use case of these attributes is to make the resulting executable aware of its own version. For example:
```nix
buildFlagsArray = [
# Note: single quotes are not needed.
"-ldflags=-X main.Version=${version} -X main.Commit=${version}"
];
```
```nix
buildFlagsArray = ''
-ldflags=
-X main.Version=${version}
-X main.Commit=${version}
'';
```

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation, should not the Go documentation be updated?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. It should be.

@Mic92 Mic92 merged commit 63a2e9c into NixOS:master Apr 21, 2021
@06kellyjac 06kellyjac deleted the cosign branch April 21, 2021 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants