-
-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
forgejo: 1.21.11-1 -> 7.0.0 #306341
forgejo: 1.21.11-1 -> 7.0.0 #306341
Conversation
The hash is leaking into the api version. Trying to track this down.
|
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 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
also, I would prefer if we could drop that |
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(
|
65ef3b6
to
6c359d6
Compare
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 |
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 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 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, |
Can you elaborate what you mean by "fixing the test obscured it"?
I am heavily against that. In Think 3206d85. I don't consider
I am in favor of maintaining multiple releases, because we have to care about But how about moving the discussion to a dedicated issue, away from this PR? I hope my response doesn't seem too harsh :( |
Yes I did, but retesting it now does show that it works as you suggested. Apparently I missed something previously.
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
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'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.
👍
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. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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 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 |
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 nixfmtmigration. As always I'm willing to roll these back if requested.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.