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

gitlab: 13.6.0 -> 13.6.1 #104689

Merged
merged 6 commits into from Nov 29, 2020
Merged

gitlab: 13.6.0 -> 13.6.1 #104689

merged 6 commits into from Nov 29, 2020

Conversation

@petabyteboy
Copy link
Member

@petabyteboy petabyteboy commented Nov 23, 2020

This is also the first time we used a new version of vgo2nix. This new
version adds a modulesDir attribute to each entry in deps.nix.

Motivation for this change
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.
@petabyteboy petabyteboy requested a review from flokli Nov 23, 2020
@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Nov 23, 2020

And it works with /vN in package path https://github.com/NixOS/nixpkgs/pull/104689/files#diff-f289978418485dbb1c3d7ccfa2f2e6a8cabc509a29e096f91c1f5d99a6654055R734. I could change it that it does not generate the empty moduleDir but I am not sure if it is worth it.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Nov 23, 2020

Result of nixpkgs-review pr 104689 run on x86_64-linux 1

5 packages built:
  • gitaly
  • gitlab
  • gitlab-ee
  • gitlab-shell
  • gitlab-workhorse

@ajs124
Copy link
Member

@ajs124 ajs124 commented Nov 24, 2020

@petabyteboy how did you test this? Did you deploy this anywhere? Did you try opening a MR against a repository?

As far as I can tell, 13.6.0 is missing gitaly-git2go, so every MR hangs at "Checking if merge request can be merged…".

@jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 24, 2020

@jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 24, 2020

@GrahamcOfBorg test gitlab

@@ -19,14 +19,14 @@ let
};
};
in buildGoPackage rec {
Copy link
Contributor

@jonringer jonringer Nov 24, 2020

I noticed they have a go.mod now. can we ditch the 300+ line deps.nix's ? https://gitlab.com/gitlab-org/gitaly/-/blob/master/go.mod

Copy link
Member Author

@petabyteboy petabyteboy Nov 24, 2020

I think people have different opinions on which Go tooling should be used. I personally don't care so I changed it to buildGoModule and removed the deps.nix.

Copy link
Member Author

@petabyteboy petabyteboy Nov 26, 2020

Oh nevermind I forgot about other Go components of GitLab, I'll update this PR to remove all deps.nix files from GitLab components.

@dasJ
Copy link
Member

@dasJ dasJ commented Nov 24, 2020

The problem ajs outlined comes from the lack of GOFLAGS = "-tags=static,system_libgit2"; in the gitaly package. Which in turn leads to missing native deps that have to be added.

@dasJ
Copy link
Member

@dasJ dasJ commented Nov 24, 2020

Full patch from our config repo:

diff --git a/4pkgs/gitlab/gitaly/default.nix b/4pkgs/gitlab/gitaly/default.nix
index 309e9900..48271faa 100644
--- a/4pkgs/gitlab/gitaly/default.nix
+++ b/4pkgs/gitlab/gitaly/default.nix
@@ -1,5 +1,7 @@
-{ stdenv, fetchFromGitLab, fetchFromGitHub, buildGoPackage, ruby,
-  bundlerEnv, pkgconfig, libgit2_0_27 }:
+{ stdenv, fetchFromGitLab, fetchFromGitHub, buildGoPackage, ruby
+, bundlerEnv, pkgconfig
+# libgit2 + dependencies
+, libgit2, openssl, zlib, pcre, http-parser }:

 let
   rubyEnv = bundlerEnv rec {
@@ -30,13 +32,14 @@ in buildGoPackage rec {
   };

   goPackagePath = "gitlab.com/gitlab-org/gitaly";
+  GOFLAGS = "-tags=static,system_libgit2";

   passthru = {
     inherit rubyEnv;
   };

   nativeBuildInputs = [ pkgconfig ];
-  buildInputs = [ rubyEnv.wrappedRuby libgit2_0_27 ];
+  buildInputs = [ rubyEnv.wrappedRuby libgit2 openssl zlib pcre http-parser ];
   goDeps = ./deps.nix;
   preBuild = "rm -rf go/src/gitlab.com/gitlab-org/labkit/vendor";

@petabyteboy petabyteboy force-pushed the feature/gitlab-13-6-1 branch from 6e48281 to e823cdd Nov 24, 2020
@petabyteboy
Copy link
Member Author

@petabyteboy petabyteboy commented Nov 24, 2020

Thanks a lot for the hint. I had to set buildFlags instead of GOFLAGS, but it did fix the issue.

@petabyteboy
Copy link
Member Author

@petabyteboy petabyteboy commented Nov 24, 2020

@petabyteboy how did you test this? Did you deploy this anywhere? Did you try opening a MR against a repository?

I deployed this and tested that git pull / git push work fine on a test repo. I will try to create a MR as well when submitting future updates, but maybe someone else (you?) can test it as well before it gets merged.

@ofborg ofborg bot requested a review from kalbasit Nov 24, 2020
@petabyteboy petabyteboy requested a review from jonringer Nov 24, 2020
talyz
talyz approved these changes Nov 26, 2020
Copy link
Contributor

@talyz talyz left a comment

Works well in my manual testing (creating a repo, adding an ssh key, pushing commits, opening and merging an MR).

Just a thought - maybe the Requires= directive on the gitaly service should be changed to BindsTo=? This creates a stronger bond between them and gitaly will be stopped if gitlab fails for any reason, not just because it's manually stopped. I know this isn't really related to the PR, but it's such a small thing that it doesn't feel worth opening a separate PR over it.

@petabyteboy petabyteboy force-pushed the feature/gitlab-13-6-1 branch 2 times, most recently from 643d7b0 to 811d374 Nov 26, 2020
@petabyteboy petabyteboy force-pushed the feature/gitlab-13-6-1 branch from 811d374 to 81aff9f Nov 26, 2020
@ofborg ofborg bot requested a review from talyz Nov 26, 2020
talyz
talyz approved these changes Nov 26, 2020
@flokli
Copy link
Contributor

@flokli flokli commented Nov 29, 2020

Let's get this in - we can still open followup PRs later.

Can you backport this to 20.09 as well? #104896 should be trickling through staging-20.09 soon.

@flokli flokli merged commit a623bc0 into NixOS:master Nov 29, 2020
19 checks passed
19 checks passed
@github-actions[bot]
tests
Details
@github-actions[bot]
action
Details
@ofborg[bot]
Evaluation Performance Report Evaluator Performance Report
Details
@github-actions[bot]
Wait for ofborg
Details
@ofborg[bot]
gitaly, gitaly.passthru.tests, gitlab, gitlab-shell, gitlab-shell.passthru.tests, gitlab-workhorse, gitlab-workhorse.passthru.tests, gitlab.passthru.tests on aarch64-linux Success
Details
@ofborg[bot]
gitaly, gitaly.passthru.tests, gitlab, gitlab-shell, gitlab-shell.passthru.tests, gitlab-workhorse, gitlab-workhorse.passthru.tests, gitlab.passthru.tests on x86_64-linux Success
Details
@ofborg[bot]
grahamcofborg-eval ^.^!
Details
@ofborg[bot]
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
@ofborg[bot]
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
@ofborg[bot]
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="81aff9f"; rev="81aff9f41142e39e3b20865396ca7343db29e82d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
@ofborg[bot]
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="81aff9f"; rev="81aff9f41142e39e3b20865396ca7343db29e82d"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="81aff9f"; rev="81aff9f41142e39e3b20865396ca7343db29e82d"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="81aff9f"; rev="81aff9f41142e39e3b20865396ca7343db29e82d"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="81aff9f"; rev="81aff9f41142e39e3b20865396ca7343db29e82d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="81aff9f"; rev="81aff9f41142e39e3b20865396ca7343db29e82d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="81aff9f"; rev="81aff9f41142e39e3b20865396ca7343db29e82d"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
@ofborg[bot]
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@dasJ
Copy link
Member

@dasJ dasJ commented Dec 23, 2020

Is the update script also updating vendorSha256? Doesn't really seem like it to me…

@petabyteboy
Copy link
Member Author

@petabyteboy petabyteboy commented Dec 24, 2020

True, it doesn't update vendorSha256. I checked each one individually for 13.6.1, but we should add something for that to the update script.

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

7 participants