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

docs/go: Document respected attributes better #97856

Merged
merged 1 commit into from Sep 24, 2020
Merged

docs/go: Document respected attributes better #97856

merged 1 commit into from Sep 24, 2020

Conversation

doronbehar
Copy link
Contributor

@doronbehar doronbehar commented Sep 12, 2020 •
edited

Motivation for this change

Often, both for shadowfox and for pistol as well (see this), I experience issues setting -ldflags for go builds. I debugged this a bit, and for shadowfox, the package in test, go install was called with my -ldflags (set from buildFlagsArray), but something unclear was wrong with the quoting, and either go install failed or link (Go's internal linker seen called if you use NIX_DEBUG = 3) didn't use it from some reason.

I had easy success with this new option for gotify-cli and shadowfox-updater - no double or mixed quotes usage.

It's hard to get ldflags / buildFlagsArray right, this PR improves the docs on the matter.

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.

@zowoq
Copy link
Contributor

zowoq commented Sep 12, 2020

Having two ways of setting these is confusing. I don't see that this is needed. Also this would mean buildGoModule handles ldflags differently from buildGoPackage.

@zowoq
Copy link
Contributor

zowoq commented Sep 12, 2020

Perhaps we should remove buildFlags and only use buildFlagsArray?

@doronbehar
Copy link
Contributor Author

Having two ways of setting these is confusing.

I think that having both buildFlags and buildFlagsArray is just as confusing. And I had no success using either, with buildFlags I felt even further from success.

Also this would mean buildGoModule handles ldflags differently from buildGoPackage.

I couldn't find any reference of ldflags used in buildGoPackage. Please correct me if my observation is wrong.

I don't see that this is needed

The reality shows that sometimes it's super hard to get buildFlagsArray right so that the correct ldflags are used. Maybe something else is wrong there - maybe it's wrong shell quoting or something, I really can't put my finger on it.

@zowoq let's put this to the test: Let's take gotify-cli as an example. Currently, $(nix-build -A gotify-cli)/bin/gotify --version reports:

Version:   unknown
Commit:    unknown
BuildDate: unknown

Tell me if there's something wrong with this patch:

diff --git i/pkgs/tools/misc/gotify-cli/default.nix w/pkgs/tools/misc/gotify-cli/default.nix
index 9e9f3c84782..1b7f0fc9d57 100644
--- i/pkgs/tools/misc/gotify-cli/default.nix
+++ w/pkgs/tools/misc/gotify-cli/default.nix
@@ -19,6 +19,10 @@ buildGoModule rec {
     mv $out/bin/cli $out/bin/gotify
   '';
 
+  buildFlagsArray = [
+    "-ldflags='-X main.Version=${version} -X main.Commit=${version}'"
+  ];
+
   meta = with lib; {
     license = licenses.mit;
     homepage = "https://github.com/gotify/cli";

Why does applying this patch doesn't make $(nix-build -A gotify-cli)/bin/gotify --version spit the right variables?? If you think it's possible to achieve, please tell me how! You can use NIX_DEBUG=6 to see both the bash commands being run by the builder, and the commands link is running when these flags are set.

@doronbehar
Copy link
Contributor Author

doronbehar commented Sep 12, 2020 •
edited

BTW that inline patch for gotify-cli uses almost verbatim what's used for gotify-server:

nixpkgs/pkgs/servers/gotify/default.nix

Lines 51 to 53 in 1435f5b

buildFlagsArray = [
"-ldflags='-X main.Version=${version} -X main.Mode=prod'"
];

And there it does work.

@zowoq
Copy link
Contributor

zowoq commented Sep 12, 2020

I couldn't find any reference of ldflags used in buildGoPackage. Please correct me if my observation is wrong.

buildGoPackage has buildFlags and buildFlagsArray so buildGoModule will have different handling.

@zowoq
Copy link
Contributor

zowoq commented Sep 12, 2020

Doesn't need the single quotes.

diff --git a/pkgs/tools/misc/gotify-cli/default.nix b/pkgs/tools/misc/gotify-cli/default.nix
index 9e9f3c84782..355bc34262f 100644
--- a/pkgs/tools/misc/gotify-cli/default.nix
+++ b/pkgs/tools/misc/gotify-cli/default.nix
@@ -19,6 +19,10 @@ buildGoModule rec {
     mv $out/bin/cli $out/bin/gotify
   '';
 
+  buildFlagsArray = [
+    "-ldflags=-X main.Version=${version} -X main.Commit=${version}"
+  ];
+
   meta = with lib; {
     license = licenses.mit;
     homepage = "https://github.com/gotify/cli";

@doronbehar
Copy link
Contributor Author

buildGoPackage has buildFlags and buildFlagsArray so buildGoModule will have different handling.

I'm not sure I understand the concern / issue, but buildGoModule also uses both buildFlags and buildFlagsArray. Perhaps it'd be nice to do the same thing I done for buildGoModule, but for buildGoPackage as well.

@zowoq
Copy link
Contributor

zowoq commented Sep 12, 2020

This style works as well:

diff --git a/pkgs/tools/misc/gotify-cli/default.nix b/pkgs/tools/misc/gotify-cli/default.nix
index 9e9f3c84782..fde9dfb9f14 100644
--- a/pkgs/tools/misc/gotify-cli/default.nix
+++ b/pkgs/tools/misc/gotify-cli/default.nix
@@ -19,6 +19,12 @@ buildGoModule rec {
     mv $out/bin/cli $out/bin/gotify
   '';
 
+  buildFlagsArray = ''
+    -ldflags=
+    -X main.Version=${version}
+    -X main.Commit=${version}
+  '';
+
   meta = with lib; {
     license = licenses.mit;
     homepage = "https://github.com/gotify/cli";

@doronbehar
Copy link
Contributor Author

DAMN. I would have never guessed it by myself, that quotes for spaces are not needed. This should be documented. I'm turning over the PR to a docs PR.

zowoq
zowoq reviewed Sep 13, 2020
View reviewed changes
doc/languages-frameworks/go.xml Outdated
@@ -152,7 +152,7 @@ deis = buildGoPackage rec {
</callout>
<callout arearefs='ex-buildGoPackage-5'>
<para>
<varname>buildFlags</varname> is a list of flags passed to the go build command.
<varname>buildFlags</varname> is a list of flags passed to the go build command, explained with more examples <link xlink:href="#ssec-go-common-attributes">here</link>.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd remove this line and line 122 entirely and just have "Attributes used by the builders" section otherwise it still looks to be specific to buildGoPackage.

Later we could de-dupe subPackages, deleteVendor and add other common attrs (e.g. excludedPackages).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the commonly respected attributes from the examples to a common section where all of them are explained.

zowoq
zowoq reviewed Sep 13, 2020
View reviewed changes
doc/languages-frameworks/go.xml Outdated
@@ -221,4 +221,34 @@ done
</screen>
</para>
</section>
<section xml:id="ssec-go-common-attributes">
<title>Attributes used by the builders</title>
<para>Both <link xlink:href="#ssec-go-modules"><varname>buildGoModule</varname></link> and <link xlink:href="#ssec-go-modules"><varname>buildGoPackage</varname></link> Read the attributes <varname>buildFlagsArray</varname> and <varname>buildFlags</varname> to set build flags supported by <varname>go</varname>'s commands such as <varname>go install</varname> and <varname>go build</varname>. Usually, you'd want to use one of the attributes to make the resulting executable aware of it's own version, or alike. For example:</para>
Copy link
Contributor

@zowoq zowoq Sep 13, 2020 •
edited

Choose a reason for hiding this comment

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

Maybe remove buildFlags and just have buildFlagsArray listed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it'd be a lie to not mention buildFlags at all, unless we also remove it from the builder. However, we can recommend in the docs to use the array.

@doronbehar doronbehar changed the title buildGoModule: support -ldflags directly docs/go: Document respected attributes better Sep 13, 2020
@doronbehar doronbehar mentioned this pull request Sep 13, 2020
Open
@doronbehar doronbehar mentioned this pull request Sep 13, 2020
Closed
zowoq
zowoq reviewed Sep 15, 2020
View reviewed changes
doc/languages-frameworks/go.xml Outdated Show resolved Hide resolved
doc/languages-frameworks/go.xml Outdated Show resolved Hide resolved
doc/languages-frameworks/go.xml Outdated Show resolved Hide resolved
doc/languages-frameworks/go.xml Outdated Show resolved Hide resolved
doc/languages-frameworks/go.xml Outdated
-X main.Version=${version}
-X main.Commit=${version}
'';
</programlisting>
Copy link
Contributor

Choose a reason for hiding this comment

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

Indentation is off.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

doc/languages-frameworks/go.xml Outdated Show resolved Hide resolved
doc/languages-frameworks/go.xml Outdated
Comment on lines 214 to 216
<para>
This form should also work:
</para>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<para>
This form should also work:
</para>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍.

@doronbehar
Copy link
Contributor Author

Squashed all changes.

@doronbehar @zowoq
docs/go: Add examples for and explain buildFlags
2b35327 
Move common attributes treated by both buildGoModule and buildGoPackage
to a separate section, out of the examples' "callouts".

Co-authored-by: zowoq <59103226+zowoq@users.noreply.github.com>
@zowoq
Copy link
Contributor

zowoq commented Sep 24, 2020

Thanks @doronbehar!

@zowoq zowoq merged commit 5819bca into NixOS:master Sep 24, 2020
@doronbehar doronbehar mentioned this pull request Oct 8, 2020
Closed
@doronbehar doronbehar deleted the misc/buildGoModule branch March 2, 2023 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zowoq zowoq zowoq left review comments

@kalbasit kalbasit Awaiting requested review from kalbasit

@Mic92 Mic92 Awaiting requested review from Mic92

Assignees
No one assigned
Labels
6.topic: golang 8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@doronbehar @zowoq