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

fastly: init at 3.1.0 #179467

Merged
merged 1 commit into from
Jun 29, 2022
Merged

fastly: init at 3.1.0 #179467

merged 1 commit into from
Jun 29, 2022

Conversation

ereslibre
Copy link
Member

Description of changes

Supersedes #170077

I could not make the shell completions to work on all my environments. I managed to make it work for x86_64-darwin but not for x86_64-linux with weird errors about the fastly binary not being found.

I can look into that, but in the meantime we have a basic working fastly binary.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ereslibre ereslibre mentioned this pull request Jun 28, 2022
13 tasks
@ereslibre
Copy link
Member Author

/cc @shyim @fabaff

@azahi
Copy link
Member

azahi commented Jun 28, 2022

Why not just use buildGoModule and then generate completions with something like this? I got a feeling that all that tarball juggling could be avoided.

@ereslibre
Copy link
Member Author

Why not just use buildGoModule and then generate completions with something like this? I got a feeling that all that tarball juggling could be avoided.

It's a tarball that contains a binary.

@azahi
Copy link
Member

azahi commented Jun 28, 2022

It's a tarball that contains a binary.

It'll be preferable to build the package from source instead of just using a prebuilt binary. We had a discussion about that recently. I think building it would be relatively easy with buildGoModule.

@ereslibre
Copy link
Member Author

In any case, if someone is interested and finds what's going on, this is the diff on top of this PR to generate the completions:

diff --git a/pkgs/misc/fastly/default.nix b/pkgs/misc/fastly/default.nix
index a42c9b26976..bf59fee42ec 100644
--- a/pkgs/misc/fastly/default.nix
+++ b/pkgs/misc/fastly/default.nix
@@ -45,6 +45,12 @@ stdenv.mkDerivation {
     runHook postInstall
   '';

+  postInstall = ''
+    $out/bin/fastly --completion-script-bash > fastly.bash
+    $out/bin/fastly --completion-script-zsh > fastly.zsh
+    installShellCompletion fastly.{bash,zsh}
+  '';
+
   meta = with lib; {
     description = "Command line tool for interacting with the Fastly API";
     license = licenses.asl20;

When trying to build it on x86_64-linux I get:

...
no Makefile, doing nothing
installing
/nix/store/n4rm9iyf5prbgxzjhv8gsxmxcxprga9x-stdenv-linux/setup: line 86: /nix/store/b099q1nr2lh5c8n23rsjcrp0zx7jv7w1-fastly-3.1.0/bin/fastly: No such file or directory
error: builder for '/nix/store/pb6qg76izdd16b6zkh2jmrxg8aj40y0n-fastly-3.1.0.drv' failed with exit code 127

I can change it to use buildGoModule but there's something odd here still... this very same derivation works just fine under x86_64-darwin.

@ereslibre
Copy link
Member Author

It'll be preferable to build the package from source instead of just using a prebuilt binary. We had a discussion about that #178896 (comment). I think building it would be relatively easy with buildGoModule

Yup, I can give it a shot.

@ereslibre
Copy link
Member Author

@azahi: updated to use buildGoModule; much better :)

@ofborg ofborg bot requested a review from kalbasit June 29, 2022 07:52
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Jun 29, 2022
Copy link
Member

@azahi azahi left a comment

Choose a reason for hiding this comment

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

You've forgot to specify ldflags which upstream adds. You can take a look at this or this for the reference.

pkgs/misc/fastly/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from shyim June 29, 2022 08:49
@ereslibre
Copy link
Member Author

Please, don't merge yet

@bobby285271 bobby285271 added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Jun 29, 2022
Copy link
Member

@06kellyjac 06kellyjac left a comment

Choose a reason for hiding this comment

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

Change mentioned in the existing thread, also:

PR title and commit need the v prefix removed

@ereslibre ereslibre force-pushed the add-fastly branch 2 times, most recently from bd138be to b561e6f Compare June 29, 2022 12:32
@ereslibre ereslibre changed the title fastly: init at v3.1.0 fastly: init at 3.1.0 Jun 29, 2022
@ofborg ofborg bot requested a review from shyim June 29, 2022 12:36
@ereslibre ereslibre requested review from 06kellyjac and azahi June 29, 2022 12:52
@ereslibre
Copy link
Member Author

Ok, this is ready now. Thanks for the pointers :)

Copy link
Member

@shyim shyim left a comment

Choose a reason for hiding this comment

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

Thanks!

@ofborg ofborg bot requested a review from shyim June 29, 2022 13:57
@ereslibre ereslibre force-pushed the add-fastly branch 2 times, most recently from f933b7a to 9abb3bb Compare June 29, 2022 14:03
@ereslibre ereslibre requested a review from 06kellyjac June 29, 2022 14:19
@ereslibre ereslibre force-pushed the add-fastly branch 2 times, most recently from 53e272e to ba38c08 Compare June 29, 2022 14:38
@SuperSandro2000 SuperSandro2000 merged commit 4a2a697 into NixOS:master Jun 29, 2022
];
preBuild = ''
ldflags+=" -X github.com/fastly/cli/pkg/revision.GitCommit=$(cat COMMIT)"
ldflags+=" -X 'github.com/fastly/cli/pkg/revision.GoVersion=$(go version)'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Upstream's HEAD has a fix for GoVersion handling introduced in fastly/cli@8ef2fd9.

In the next release, you can eliminate running go version and passing the output in favor of passing GoHostOS and GoHostArch in ldflags and letting it render a proper version.

{lib, fetchFromGithub, [...], go} : 


[...]


ldflags = [
  ...
  "-X github.com/fastly/cli/pkg/revision.GoHostOS=${go.GOOS}"
  "-X github.com/fastly/cli/pkg/revision.GoHostArch=${go.GOARCH}"
];

diff --git a/pkgs/misc/fastly/default.nix b/pkgs/misc/fastly/default.nix
index 1a376271d18..2028f3f9bcf 100644
--- a/pkgs/misc/fastly/default.nix
+++ b/pkgs/misc/fastly/default.nix
@@ -1,14 +1,14 @@
-{ lib, fetchFromGitHub, installShellFiles, buildGoModule }:
+{ lib, fetchFromGitHub, installShellFiles, buildGoModule, go }:

 buildGoModule rec {
   pname = "fastly";
-  version = "3.1.0";
+  version = "HEAD";

   src = fetchFromGitHub {
     owner = "fastly";
     repo = "cli";
-    rev = "v${version}";
-    sha256 = "sha256-Su4ZwiuI+pMoLAGhc3dWcwgcfwe5cZGTg8kEnpM4JbA=";
+    rev = "HEAD";
+    sha256 = "sha256-hVcL7kK+UL3HCrtHU4Jq16DcItWBBo4dnhy7PzNrD+I=";
     # The git commit is part of the `fastly version` original output;
     # leave that output the same in nixpkgs. Use the `.git` directory
     # to retrieve the commit SHA, and remove the directory afterwards,
@@ -34,10 +34,11 @@ buildGoModule rec {
     "-w"
     "-X github.com/fastly/cli/pkg/revision.AppVersion=v${version}"
     "-X github.com/fastly/cli/pkg/revision.Environment=release"
+    "-X github.com/fastly/cli/pkg/revision.GoHostOS=${go.GOOS}"
+    "-X github.com/fastly/cli/pkg/revision.GoHostArch=${go.GOARCH}"
   ];
   preBuild = ''
     ldflags+=" -X github.com/fastly/cli/pkg/revision.GitCommit=$(cat COMMIT)"
-    ldflags+=" -X 'github.com/fastly/cli/pkg/revision.GoVersion=$(go version)'"
   '';

   postInstall = ''
% nix run .#fastly -- version
[...]
Fastly CLI version vHEAD (5b0d8ee)
Built with go version go1.17.11 linux/amd64

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, thanks @bdd!

Copy link
Member

Choose a reason for hiding this comment

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

GoVersion will continue to work too but yes, the more nix-ie way would be to pull from go.

As a note those are the wrong values. It needs to be go.GOHOSTOS and °go.GOHOSTARCH` for the build host details rather than build target details

https://github.com/NixOS/nixpkgs/blob/nixos-22.05/pkgs/development/compilers/go/1.17.nix#L201

GOOS and GOARCH for the target (running) system are available in the runtime package. This was covered in the other comment and PR

@ereslibre ereslibre deleted the add-fastly branch June 29, 2022 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants