forgejo: 1.21.11-1 -> 7.0.0

[adamcstephens]

Description of changes

https://codeberg.org/forgejo/forgejo/src/branch/forgejo/RELEASE-NOTES.md#7-0-0

https://forgejo.org/2024-04-release-v7-0/

Also did the by-name and nixfmt migration. As always I'm willing to roll these back if requested.

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• 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/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 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
• Fits CONTRIBUTING.md.

---

Add a 👍 reaction to pull requests you find important.

[adamcstephens]

The hash is leaking into the api version. Trying to track this down.

AssertionError: /api/forgejo/v1/version should not return 'development' but should contain a gitea compatibility version string. Got '73c190a+gitea-1.22.0' instead.

[pyrox0]

The hash is leaking into the api version. Trying to track this down.

AssertionError: /api/forgejo/v1/version should not return 'development' but should contain a gitea compatibility version string. Got '73c190a+gitea-1.22.0' instead.

Not sure why the hash is leaking, but the test error is caused by the test at https://github.com/NixOS/nixpkgs/blob/master/nixos/tests/forgejo.nix#L144 checking for the string -gitea- instead of +gitea-, as they've changed the compatibility version string format. Fixing this causes it to fail at a later test.

Also, the code at https://github.com/adamcstephens/nixpkgs/blob/65ef3b6d65bb97450fd2cabef1335bbd3daffa06/pkgs/by-name/fo/forgejo/package.nix#L50-L65 should be able to be deleted. As noted in the comment, it's now included in the Makefile for this version, though leaveDotGit may need to be kept enabled, as the Makefile relies on being able to use git to get the version if it can't find it via another method.

[emilylange]

diff --git a/nixos/tests/forgejo.nix b/nixos/tests/forgejo.nix
index 8b9ee46ff5d3..15d603ea28a1 100644
--- a/nixos/tests/forgejo.nix
+++ b/nixos/tests/forgejo.nix
@@ -141,7 +141,7 @@ let
         assert "BEGIN PGP PUBLIC KEY BLOCK" in server.succeed("curl http://localhost:3000/api/v1/signing-key.gpg")
 
         api_version = json.loads(server.succeed("curl http://localhost:3000/api/forgejo/v1/version")).get("version")
-        assert "development" != api_version and "-gitea-" in api_version, (
+        assert "development" != api_version and "+gitea-" in api_version, (
             "/api/forgejo/v1/version should not return 'development' "
             + f"but should contain a gitea compatibility version string. Got '{api_version}' instead."
         )
diff --git a/pkgs/by-name/fo/forgejo/package.nix b/pkgs/by-name/fo/forgejo/package.nix
index 30c22ec8af1c..6ffad74b6fd0 100644
--- a/pkgs/by-name/fo/forgejo/package.nix
+++ b/pkgs/by-name/fo/forgejo/package.nix
@@ -46,21 +46,18 @@ buildGoModule rec {
     owner = "forgejo";
     repo = "forgejo";
     rev = "v${version}";
-    hash = "sha256-FpNMCSZSMiRtxQ4lNfOVt70kHe1SdHZr6F5XOdJz/hM=";
+    hash = "sha256-+M+Qr4fYLTOEhVu9HfJ7DtGqbWrKcsZszed6PfoZuFI=";
     # Forgejo has multiple different version strings that need to be provided
     # via ldflags.  main.ForgejoVersion for example is a combination of a
     # hardcoded gitea compatibility version string (in the Makefile) and
     # git describe and is easiest to get by invoking the Makefile.
     # So we do that, store it the src FOD to then extend the ldflags array
     # in preConfigure.
-    # The `echo -e >> Makefile` is temporary and already part of the next
-    # major release.  Furthermore, the ldflags will change next major release
-    # and need to be updated accordingly.
     leaveDotGit = true;
+    deepClone = true;
     postFetch = ''
       cd "$out"
-      echo -e 'show-version-full:\n\t@echo ''${FORGEJO_VERSION}' >> Makefile
-      make show-version-full > FULL_VERSION
+      make show-version-api > FULL_VERSION
       find "$out" -name .git -print0 | xargs -0 rm -rf
     '';
   };
@@ -98,7 +95,7 @@ buildGoModule rec {
   ];
 
   preConfigure = ''
-    export ldflags+=" -X code.gitea.io/gitea/routers/api/forgejo/v1.ForgejoVersion=$(cat FULL_VERSION) -X main.ForgejoVersion=$(cat FULL_VERSION)"
+    export ldflags+=" -X main.ForgejoVersion=$(cat FULL_VERSION)"
   '';
 
   preBuild = ''

There is also another failing test command. Will look into that one as well.

[emilylange]

also, I would prefer if we could drop that nix-fmt rfc commit, since it's planned to do that treewide, which has the advantage of being added to .git-blame-ignore-revs

[emilylange]

There is also another failing test command. Will look into that one as well.

needs

diff --git a/nixos/tests/forgejo.nix b/nixos/tests/forgejo.nix
index 8b9ee46ff5d3..abd37224ba14 100644
--- a/nixos/tests/forgejo.nix
+++ b/nixos/tests/forgejo.nix
@@ -152,7 +152,7 @@ let
         )
         server.succeed(
             "su -l forgejo -c 'GITEA_WORK_DIR=/var/lib/forgejo gitea admin user create "
-            + "--username test --password totallysafe --email test@localhost'"
+            + "--username test --password totallysafe --email test@localhost --must-change-password=false'"
         )
 
         api_token = server.succeed(

--must-change-password=false vs --must-change-password false is a bit funky, so please make sure to include the = for now.

❯ nix-build -A forgejo.tests
/nix/store/z08gbnc17v8qkd3syq9iv4013dndh7lk-vm-test-run-forgejo-mysql
/nix/store/aa0h61x1bvml144x1wd66y0s2yrivqgn-vm-test-run-forgejo-postgres
/nix/store/0rzrzak49zk4wycc231vf81zr7sbqqx8-vm-test-run-forgejo-sqlite3

[adamcstephens]

Thanks Emily! The version fix still wasn't sufficient unfortunately, and fixing the test obscured it. :/ I improved the test so we can verify the returned version matches the package version.

I also hardcoded the version from our package instead of using their git detection. The deepClone is slow and this allows for completely removing it in favor of a simple fetcher.

Setting the oddly named $GITEA_VERSION allows for this conditional to give us the proper version.
https://codeberg.org/forgejo/forgejo/src/commit/73c190af4c13a09453b878e255c31cd70ddd5ab9/Makefile#L92-L93

[adamcstephens]

This release is considered an LTS and they will now have two releases moving forward. Do we want to stay on this LTS or support multiple versions when the next version comes out?

[pyrox0]

I propose the following, if multiple version support is wanted: Have a forgejo_<major version>, i.e forgejo_7attribute for each LTS, and then just forgejo for the latest release(LTS or not).

Going by the release schedule, there will be a 3-month overlap every year where there are multiple supported LTS releases, and thus something like having a single forgejo_lts attribute alongside the regular forgejo attribute wouldn't be feasible.

I do think that at least supporting the most recent LTS release is a good idea, but I don't really have an idea of how much value there would be in supporting more than that.

Also, as an aside, would you please add me to the maintainers list for this package? My committer attribute is the same as my Github username, pyrox0.

[emilylange]

Thanks Emily! The version fix still wasn't sufficient unfortunately, and fixing the test obscured it. :/ I improved the test so we can verify the returned version matches the package version.

Can you elaborate what you mean by "fixing the test obscured it"?
Did you apply the whole diff I send you or only parts of it?

I also hardcoded the version from our package instead of using their git detection. The deepClone is slow and this allows for completely removing it in favor of a simple fetcher.

I am heavily against that.
We specifically went that route in #300981, and I thought I had your approval on that.

In nixos-23.11 we had multiple version bumps that used commit revs instead of release tags.
git describe would have helped a lot.

Think 3206d85.

I don't consider deepClone being slow all that bad, since it isn't user-facing.
But let me look into making it faster, downloading only the bare minimum for an actually helpful git describe to work, if that's alright with you.

This release is considered an LTS and they will now have two releases moving forward. Do we want to stay on this LTS or support multiple versions when the next version comes out?

I propose the following, if multiple version support is wanted: Have a forgejo_<major version>, i.e forgejo_7attribute for each LTS, and then just forgejo for the latest release(LTS or not).
[...]

I am in favor of maintaining multiple releases, because we have to care about nixos-stable (23.11, 24.05, ...) as well, not just nixos-unstable.

But how about moving the discussion to a dedicated issue, away from this PR?
We don't need to action right now, especially with #303285 in mind.

---

I hope my response doesn't seem too harsh :(

[adamcstephens]

Can you elaborate what you mean by "fixing the test obscured it"? Did you apply the whole diff I send you or only parts of it?

Yes I did, but retesting it now does show that it works as you suggested. Apparently I missed something previously.

I also hardcoded the version from our package instead of using their git detection. The deepClone is slow and this allows for completely removing it in favor of a simple fetcher.

I am heavily against that. We specifically went that route in #300981, and I thought I had your approval on that.

I did approve that and stand by it, but feel a bit differently today. One of the problems with relying on upstream's git revs is that while we get the commit hash we lose the major version. You can see that above in 73c190a+gitea-1.22.0. This also seems less than ideal to me, but if you feel strongly a hash-only version is better I'll go along. Ideally we'd keep both the version and the hash.

In nixos-23.11 we had multiple version bumps that used commit revs instead of release tags. git describe would have helped a lot.

Won't this be somewhat mooted by the separate release cycle for LTS? Do you know if upstream will be cutting releases now instead of just backporting to a branch?

I don't consider deepClone being slow all that bad, since it isn't user-facing. But let me look into making it faster, downloading only the bare minimum for an actually helpful git describe to work, if that's alright with you.

I'm open to reverting. It adds a minute or so for any rebuild, and felt unnecessary since this solution exists and is cleaner.

The more I've looked at the versioning in upstream, the more I question if we wouldn't be better off just coding all the version info into nixpkgs rather than trying to run it through their Makefile. I've started working on a fixing the update script and it wouldn't be much harder to also fetch the gitea-compatibility version from the repo.

But how about moving the discussion to a dedicated issue, away from this PR? We don't need to action right now, especially with #303285 in mind.

👍

I hope my response doesn't seem too harsh :(

A bit, but it's ok. I understand you probably feel like I'm stepping on your toes and I want to apologize for that. We can figure out any differences I'm sure. :)

[ivan]

I applied this PR on my x86_64 NixOS staging-next; I deployed a new Forgejo instance connected to PostgreSQL 16.2 and I didn't see issues.

[emilylange]

@adamcstephens reached out to me on matrix yesterday, and we had a nice chat where we could clarify a few things in private.

I don't have the spoons to look again into this today or tomorrow, due to the whole nix community situation and all, unfortunately.

There might be things that could be polished but ehh, that's by no means a priority.

Reverting the fetchPost+leaveDotGit thingy in favor of hardcoding it is also fine with me.
I had a think and if we ever /actually/ need it again (which is unlikely given the LTS series will have proper releases, instead of revs on a branch), we can easily re-add it again.

Thanks a lot for the private chat @adamcstephens :)

I'll dismiss my review now, so merging is no longer blocked.

I won't give this my explicit approval because I didn't look as thorough into this as I would usually do, due to lack of spoons and time.

But feel free to merge this nonetheless, please.

Thanks a lot for rebasing the patches, too!

PS: For passthru.updateScript to work with fetchPost+leaveDotGit and codeberg, Mic92/nix-update#244 is needed, which hasn't been released yet. Reverting the fetchPost+leaveDotGit thingy also makes the update script work again, without that unreleased PR, so that's even more of a win, I suppose :)

See #306341 (comment)