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

symfony-cli: add missing compilation flags #206149

Merged
merged 1 commit into from Dec 16, 2022
Merged

symfony-cli: add missing compilation flags #206149

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion pkgs/development/tools/symfony-cli/default.nix
View file
Open in desktop
Expand Up @@ -12,12 +12,20 @@ buildGoModule rec {
sha256 = "sha256-GAsyI8I+tHFMV/LqwPx2ph+w3zaqKSn9vieVQcuO+y0=";
};

ldflags = [
"-s"
"-w"
"-X main.version=${version}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Are not these two different flags?

Suggested change
"-X main.version=${version}"
"-X" "main.version=${version}"

Copy link
Author

Choose a reason for hiding this comment

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

I don’t really know if it should be considered as two different flags. If we wanted to add something like main.channel it would result in :

ldflags = [
  "-s"
  "-w"
  "-X"
  "main.version=${version}"
  "-X"
  "main.channel=${channel}"
]

instead of :

ldflags = [
  "-s"
  "-w"
  "-X main.version=${version}"
  "-X main.channel=${channel}"
]

I really don’t know what is the best for NixOS

Copy link
Contributor

Choose a reason for hiding this comment

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

The question is whether it works if you run it in shell.

The stdenv.mkDerivation does not really support lists by default so currently, it will end up being run like go install "-ldflags=-s -w -X main.version=x.y.z -X main.channel=foo":

nixpkgs/pkgs/build-support/go/module.nix

Line 211 in fbcb61b

flags+=(''${ldflags:+-ldflags="$ldflags"})

But in the future, when we switch to structured attributes, it would be good if arguments would be properly split.

];

postInstall = ''
mv $out/bin/symfony-cli $out/bin/symfony
'';

# Tests requires network access
doCheck = false;
checkPhase = ''
$GOPATH/bin/symfony-cli
Copy link
Contributor

Choose a reason for hiding this comment

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

When overriding checkPhase, you should also provide runHook preCheck and runHook postCheck. See https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/missing-phase-hooks.md for more info.

Though, in this case, it might be better to use passthru.tests instead of replacing checkPhase so that it can be verified it works outside of the build environment (in a cleaner environment, more representative of actual users’ systems). And also so that we can re-instate doCheck when we manage to fix it.

nixpkgs/pkgs/tools/misc/osm2pgsql/default.nix

Lines 49 to 51 in 8bc6f34

passthru.tests.version = testers.testVersion {
package = finalAttrs.finalPackage;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

@jtojnar I can provide an updated PR but I would need more info on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

What info do you mean?

'';

meta = with lib; {
description = "Symfony CLI";
Expand Down