nixos/gitea: add WORK_PATH to config, fix 1.20

[bendlas]

Description of changes

Gitea and Forgejo 1.20 need a config option WORK_PATH set and will try to update the config file, which will fail on NixOS where the config is read-only.

Also add myself as maintainer to forgejo

TODO: Either test this with 1.19 or wait for 1.20 to be released before merging this.

DONE: Tested together with 1.19.4-0 from #241777

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:
  • 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/)
• 23.11 Release Notes (or backporting 23.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
• Fits CONTRIBUTING.md.

[emilylange]

Looks good to me, thanks!

But would you mind changing your gitea: add bendlas to maintainers commit message to forgejo: add bendlas to maintainers? :)

[bendlas]

But would you mind changing your gitea: add bendlas to maintainers commit message to forgejo: add bendlas to maintainers? :)

oof yes, done, thanks!

[bendlas]

I was wondering if it wouldn't be worth pulling up the extraConfig line (${optionalString (cfg.extraConfig != null) cfg.extraConfig}) to before the main settings section.

That way, a non-sectioned setting like WORK_PATH could be added via extraConfig as well. Right now such a setting would get assigned randomly to whatever the last settings section turned out to be, right?

Do you see any trade-offs here, @emilylange?

[emilylange]

From go-gitea/gitea#25330

The "work path" priority is: WORK_PATH in app.ini > cmd arg --work-path > env var GITEA_WORK_DIR > builtin default

We seem to be doing something similar with GITEA_WORK_DIR at

nixpkgs/nixos/modules/services/misc/gitea.nix

Line 625 in 687ed41

GITEA_WORK_DIR = cfg.stateDir;

and

nixpkgs/nixos/modules/services/misc/gitea.nix

Line 665 in 687ed41

GITEA_WORK_DIR = cfg.stateDir;

Assuming the env var does not try to update the specified app.ini, this might be a more suitable place.
Or, alternatively, a lib.mkDefaulted WORK_PATH = cfg.stateDir in config similar to

nixpkgs/nixos/modules/services/misc/gitea.nix

Lines 391 to 392 in 687ed41

	services.gitea.settings = {
	"cron.update_checker".ENABLED = lib.mkDefault false;

I don't know why one would want to manually override WORK_PATH when using the module, but ehh, maybe there are use-cases.
And both solutions would allow for that :)

I don't see much value changing the order of the freeform settings option and extraConfig.
But maybe I just don't quite get what you mean with that^^

[bendlas]

I don't really know what's going on either. If there is no WORK_PATH in app.ini, it will try to add it (possibly from that env var you're mentioning). That will fail when app.ini is read-only.

Maybe that could be considered a bug upstream, but I wanted to leave this here, so that we already know when doing the update.

[bendlas]

I don't see much value changing the order of the freeform settings option and extraConfig.
But maybe I just don't quite get what you mean with that^^

What I meant is: I couldn't add the WORK_PATH through extra config, because:

UNSECTIONED_CONFIG1 = value1

[section1]
SECTION_CONFIG = value2

EXTRA_CONFIG = value3

here, EXTRA_CONFIG = value3 would be considered part of [section1], even though it's meant to be unsectioned. There is also no code that could reasonably rely on this behavior, since the order of sections is essentially random and so the section that an unsectioned extraConfig is assigned to would also be random.

Also I don't think there is a way to denote a "non-section" in order to add more unsectioned values, so in my books moving the extraConfig up would be strictly an improvement, because it would allow adding unsectioned values at all.

[SuperSandro2000]

TLDR but extraConfig is deprecated and we should remove it rather sooner than later.

[bendlas]

Good to know.

Do our .ini config helpers do the unsectioned preamble?

[SuperSandro2000]

gitea 1.20 is not released yet, drafting again

[bendlas]

Not needed, see #243883

[ghost]

Huh, why was this closed? I ran into this issue today after updating and after finding this PR and locking the nixpkgs to the ref of #243883 (b61919e) the issue still happens so I think this is still needed, unless I'm missing something. Either that or remove the restriction of settings that you cant have top level non dict entries in there.

[emilylange]

I assume you are using Forgejo via services.gitea.packages and your systemd service is restarting every 90 seconds?

If so, the commit in question (b61919e) has been reverted in #244843 and should land in nixos-unstable-small and nixos-unstable shortly, which should solves this.
See https://nixpk.gs/pr-tracker.html?pr=244843
and #243883 (comment)

Also, on a related note:
Forgejo will split out of nixos/gitea (a new services.forgejo) soon, which hopefully prevents such issues in the future.
See #244866

[ghost]

No, Im using just gitea and the issue i have is gitea wanting to set the missing WORK_PATH variable in its config, but failing because its read-only and aborting immediately.

Jul 23 10:56:50 patchouli gitea[1129269]: 2023/07/23 10:56:46 cmd/web.go:170:serveInstalled() [E] Unable to update WORK_PATH=/var/lib/gitea to config "/var/lib/gitea/custom/conf/app.ini": failed to save "/var/lib/gitea/custom/conf/app.ini": open /var/lib/gitea/custom/conf/app.ini: permission denied
Jul 23 10:56:50 patchouli gitea[1129269]: You must set it manually, otherwise there might be bugs when accessing the git repositories.

Which is also why im so confused this got closed in favour of #244843 because I dont see said fix there or a mention in the upstream changelog that the behaviour changed, i dont think the service type has anything to do with what this PR originally fixed?

EDIT: as i was removing a deprecated entry from gitea.settings it still gives me that error but now stays active (so more of a warning i suppose) so im guessing this didnt cause the crash but using a deprecated setting ([indexer].UPDATE_BUFFER_LEN) did? Works for me i suppose

[emilylange]

Alright, sorry^^

[bendlas]

Huh, why was this closed? I ran into this issue today after updating and after finding this PR and locking the nixpkgs to the ref of #243883 (b61919e) the issue still happens so I think this is still needed, unless I'm missing something. Either that or remove the restriction of settings that you cant have top level non dict entries in there.

I was assuming that since @Ma27 has been successfully running the updated version, that this change here was not needed after all and did not try myself yet #243883 (comment)

I'll re-open until we figured out what's going on.

@Ma27 can you comment: do you have WORK_PATH = /var/lib/gitea in your /var/lib/gitea/custom/conf/app.ini?

[Ma27]

🤦

Oof... I really feel like being struck my Murphy's law re gitea this weekend /o\

tl;dr I get the same warning in my logs, so this PR is indeed needed. Please resolve the conflict in pkgs.forgejo and let CI run one more time, then we should be good to go with this.

First of all, apologies! Let me explain why I missed this:

• I haven't seen it in the changelogs and with gitea: 1.19.4 -> 1.20.0 #243883 both the tests and my own instance were operating normally and as I'd expect it and I haven't missed the log-line because of that.
• I misinterpreted the conversation about this issue in gitea: 1.19.4 -> 1.20.0 #243883 as hard crash of gitea and not as log entry which is why I (wrongly) assumed this issue to be resolved. Considering the "there may be bugs" warning we should definitely fix it. Also, I assumed a noticeable problem because I got bitten by a different settings issue as well (CHUNKED_UPLOAD_PATH) so I was fixed on another similar issue.

This is just an explanation, but I won't sugar-coat it, missing that issue was clearly my mistake!

@maddiethecafebabe just to make sure I'm not misunderstanding you: currently your gitea runs fine and the only issue you see is this error in the logs, correct? If that's the case (and no other problems can be observed) then it should be OK to await the next channel bump with this PR included (I'm running gitea for almost a week now without any issues). If you don't want to do that - which is perfectly fine of course: do you need assistance with adding unmerged nixpkgs patches to your deployment or can you do that on your own?

[ghost]

No worries! Yeah, the log message is marked as an error [E] but it seems to be really just a warning so indeed not urgently, it was something else that caused the crash for me that is now fixed. Thanks yall for looking into it again so quickly

[bendlas]

I remember it going into a restart loop, but that was also an earlier RC.

Merge conflict resolved.

[Ma27]

Sorry, forgot that this is still open 🤦