From 4e87e35bb535cd0442f12cc99a5e7b3d2cc05217 Mon Sep 17 00:00:00 2001 From: emilylange Date: Wed, 18 Dec 2024 02:52:01 +0100 Subject: [PATCH] feat(forgejo): offload custom forgejo package into its own repository It has been a recurring issue that flake lockfile bumps in this repo here make the forgejo patches no longer apply. The dedicated repository (nix-forgejo) solves this by not overriding the existing forgejo derivation from nixpkgs but rather having its own. Additionally, nix-forgejo pins and uses a "known good" nixpkgs revision itself, unless `pkgs` is passed on import. So if issues should arise after a flake bump, we can use that revision by modifying our import statement, or we can rollback the nix-forgejo revision itself. Moving forgejo out of tree also makes iterating on it a lot easier and opens a lot of other possibilities :) --- flake.lock | 17 ++++++ flake.nix | 3 + ...e-expensive-commit-divergence-metric.patch | 59 ------------------- ...cherry-pick-default-branch-selection.patch | 32 ---------- pkgs/forgejo/default.nix | 40 ------------- ...-expensive-repository-activity-stats.patch | 34 ----------- ...igrations-and-pull-mirrors-to-admins.patch | 53 ----------------- services/forgejo/default.nix | 5 +- 8 files changed, 23 insertions(+), 220 deletions(-) delete mode 100644 pkgs/forgejo/branch-view_remove-expensive-commit-divergence-metric.patch delete mode 100644 pkgs/forgejo/commit-view_fix-broken-and-expensive-cherry-pick-default-branch-selection.patch delete mode 100644 pkgs/forgejo/default.nix delete mode 100644 pkgs/forgejo/disable-expensive-repository-activity-stats.patch delete mode 100644 pkgs/forgejo/limit-migrations-and-pull-mirrors-to-admins.patch diff --git a/flake.lock b/flake.lock index 7dc5874..1f2cdd9 100644 --- a/flake.lock +++ b/flake.lock @@ -568,6 +568,22 @@ "type": "github" } }, + "nix-forgejo": { + "flake": false, + "locked": { + "lastModified": 1734486094, + "narHash": "sha256-WZ2vtxCItlfev7um8lBFzXtp7WYVHsDNrCnk1i8yLAU=", + "ref": "refs/heads/main", + "rev": "403b7bab1b5c37275ab946a5944c2caaf12eca78", + "revCount": 1, + "type": "git", + "url": "https://git.lix.systems/the-distro/nix-forgejo.git" + }, + "original": { + "type": "git", + "url": "https://git.lix.systems/the-distro/nix-forgejo.git" + } + }, "nix-gerrit": { "inputs": { "nixpkgs": [ @@ -809,6 +825,7 @@ "hydra", "lix" ], + "nix-forgejo": "nix-forgejo", "nix-gerrit": "nix-gerrit", "nixpkgs": "nixpkgs_2", "ofborg": "ofborg", diff --git a/flake.nix b/flake.nix index bad2325..d14c8bf 100644 --- a/flake.nix +++ b/flake.nix @@ -19,6 +19,9 @@ nix-gerrit.url = "git+https://git.lix.systems/the-distro/nix-gerrit.git?ref=refs/heads/bump-minor-3_10"; nix-gerrit.inputs.nixpkgs.follows = "nixpkgs"; + nix-forgejo.url = "git+https://git.lix.systems/the-distro/nix-forgejo.git"; + nix-forgejo.flake = false; + ofborg.url = "git+https://git.lix.systems/the-distro/ofborg.git?ref=refs/heads/vcs-generalization"; ofborg.flake = false; diff --git a/pkgs/forgejo/branch-view_remove-expensive-commit-divergence-metric.patch b/pkgs/forgejo/branch-view_remove-expensive-commit-divergence-metric.patch deleted file mode 100644 index c4bb4c2..0000000 --- a/pkgs/forgejo/branch-view_remove-expensive-commit-divergence-metric.patch +++ /dev/null @@ -1,59 +0,0 @@ -diff --git a/services/repository/branch.go b/services/repository/branch.go -index e1a313749f..5a8d823eef 100644 ---- a/services/repository/branch.go -+++ b/services/repository/branch.go -@@ -26,7 +26,6 @@ import ( - "code.gitea.io/gitea/modules/timeutil" - webhook_module "code.gitea.io/gitea/modules/webhook" - notify_service "code.gitea.io/gitea/services/notify" -- files_service "code.gitea.io/gitea/services/repository/files" - - "xorm.io/builder" - ) -@@ -129,21 +128,7 @@ func loadOneBranch(ctx context.Context, repo *repo_model.Repository, dbBranch *g - p := protectedBranches.GetFirstMatched(branchName) - isProtected := p != nil - -- var divergence *git.DivergeObject -- -- // it's not default branch -- if repo.DefaultBranch != dbBranch.Name && !dbBranch.IsDeleted { -- var err error -- divergence, err = files_service.CountDivergingCommits(ctx, repo, git.BranchPrefix+branchName) -- if err != nil { -- return nil, fmt.Errorf("CountDivergingCommits: %v", err) -- } -- } -- -- if divergence == nil { -- // tolerate the error that we cannot get divergence -- divergence = &git.DivergeObject{Ahead: -1, Behind: -1} -- } -+ divergence := &git.DivergeObject{Ahead: -1, Behind: -1} - - pr, err := issues_model.GetLatestPullRequestByHeadInfo(ctx, repo.ID, branchName) - if err != nil { -diff --git a/templates/repo/branch/list.tmpl b/templates/repo/branch/list.tmpl -index a577fed450..e102796315 100644 ---- a/templates/repo/branch/list.tmpl -+++ b/templates/repo/branch/list.tmpl -@@ -102,19 +102,6 @@ - {{end}} - - -- {{if and (not .DBBranch.IsDeleted) $.DefaultBranchBranch}} --
--
--
{{.CommitsBehind}}
-- {{/* old code bears 0/0.0 = NaN output, so it might output invalid "width: NaNpx", it just works and doesn't caues any problem. */}} --
--
--
--
{{.CommitsAhead}}
--
--
--
-- {{end}} - - - {{if not .LatestPullRequest}} diff --git a/pkgs/forgejo/commit-view_fix-broken-and-expensive-cherry-pick-default-branch-selection.patch b/pkgs/forgejo/commit-view_fix-broken-and-expensive-cherry-pick-default-branch-selection.patch deleted file mode 100644 index ae7e60f..0000000 --- a/pkgs/forgejo/commit-view_fix-broken-and-expensive-cherry-pick-default-branch-selection.patch +++ /dev/null @@ -1,32 +0,0 @@ -diff --git a/routers/web/repo/commit.go b/routers/web/repo/commit.go -index 718454e063..8fa299710c 100644 ---- a/routers/web/repo/commit.go -+++ b/routers/web/repo/commit.go -@@ -408,12 +408,6 @@ func Diff(ctx *context.Context) { - } - } - -- ctx.Data["BranchName"], err = commit.GetBranchName() -- if err != nil { -- ctx.ServerError("commit.GetBranchName", err) -- return -- } -- - ctx.HTML(http.StatusOK, tplCommitPage) - } - -diff --git a/templates/repo/commit_page.tmpl b/templates/repo/commit_page.tmpl -index c37fb46975..18c9cf18f8 100644 ---- a/templates/repo/commit_page.tmpl -+++ b/templates/repo/commit_page.tmpl -@@ -71,8 +71,8 @@ - "branchForm" "branch-dropdown-form" - "branchURLPrefix" (printf "%s/_cherrypick/%s/" $.RepoLink .CommitID) "branchURLSuffix" "" - "setAction" true "submitForm" true}} --
-- -+ -+ - -
- diff --git a/pkgs/forgejo/default.nix b/pkgs/forgejo/default.nix deleted file mode 100644 index ddb5b0e..0000000 --- a/pkgs/forgejo/default.nix +++ /dev/null @@ -1,40 +0,0 @@ -{ forgejo-lts }: - -forgejo-lts.overrideAttrs (prev: { - patches = [ - # Branch divergence calculations for a single branch may take 100-200ms on something as big - # as nixpkgs. The branch view defaults to 20 branches for each page, taking roughtly 3s to - # calculate each branch sequentially and render, while consuming a single core at 100%. - # The idea is to look into making this less expensive or async. - # But for now, to get this going, we will simply drop that metric. - ./branch-view_remove-expensive-commit-divergence-metric.patch - - # This is literally broken and eats resources for nothing of value. - # We should upstream this. - # The tl;dr is: It calculates the nearest branch for the requested commit at - # /:owner/:repo/commit/:commit to use it as the default cherry-pick target branch - # selection in a drop-down only users with commit perms can actually view and use. - # It's expensive to calculate and happens on every request to /commit/:commit. - # To add insult to injury, it's hardly of any use: The nearest branch of a commit - # will almost always be a branch that already carries the commit. The branch you - # most likely don't want to cherry-pick to. - ./commit-view_fix-broken-and-expensive-cherry-pick-default-branch-selection.patch - - # Disable various /:owner/:repo/activity/ sub-views. They are expensive, which is - # totally fine and expected. There is even proper caching in place. - # However, on a scale of nixpkgs, those calculations take ages, while, of course, - # pinning a single CPU core at 100%. - # For now, we will simply disable this feature. - # Due to the 501 status code it returns, the frontend prints a "Not implemented" - # error, saving us from patching the frontend while still providing a helpful - # user-facing error text. - # It should be noted that this particular status code has the downside of being - # in the 5xx range, meaning it will show up as such in our prometheus metrics. - ./disable-expensive-repository-activity-stats.patch - - # Migrations and pull-mirrors are something easily abused to bring a public instance to a complete halt. - # Both features can be disabled via repository.DISABLE_MIGRATIONS and mirror.ENABLE, but we want to keep - # this functionality for admins. - ./limit-migrations-and-pull-mirrors-to-admins.patch - ]; -}) diff --git a/pkgs/forgejo/disable-expensive-repository-activity-stats.patch b/pkgs/forgejo/disable-expensive-repository-activity-stats.patch deleted file mode 100644 index 555007c..0000000 --- a/pkgs/forgejo/disable-expensive-repository-activity-stats.patch +++ /dev/null @@ -1,34 +0,0 @@ -diff --git a/routers/web/web.go b/routers/web/web.go -index ee9694f41c..f55b8d6f62 100644 ---- a/routers/web/web.go -+++ b/routers/web/web.go -@@ -57,6 +57,10 @@ import ( - "github.com/prometheus/client_golang/prometheus" - ) - -+func endpointNotImplemented(ctx *context.Context) { -+ ctx.JSON(http.StatusNotImplemented, "This endpoint has been removed due to performance issues with it and as such is not longer implemented.") -+} -+ - // optionsCorsHandler return a http handler which sets CORS options if enabled by config, it blocks non-CORS OPTIONS requests. - func optionsCorsHandler() func(next http.Handler) http.Handler { - var corsHandler func(next http.Handler) http.Handler -@@ -1425,15 +1429,15 @@ func registerRoutes(m *web.Route) { - m.Get("/{period}", repo.Activity) - m.Group("/contributors", func() { - m.Get("", repo.Contributors) -- m.Get("/data", repo.ContributorsData) -+ m.Get("/data", endpointNotImplemented) - }, repo.MustBeNotEmpty, context.RequireRepoReaderOr(unit.TypeCode)) - m.Group("/code-frequency", func() { - m.Get("", repo.CodeFrequency) -- m.Get("/data", repo.CodeFrequencyData) -+ m.Get("/data", endpointNotImplemented) - }, repo.MustBeNotEmpty, context.RequireRepoReaderOr(unit.TypeCode)) - m.Group("/recent-commits", func() { - m.Get("", repo.RecentCommits) -- m.Get("/data", repo.RecentCommitsData) -+ m.Get("/data", endpointNotImplemented) - }, repo.MustBeNotEmpty, context.RequireRepoReaderOr(unit.TypeCode)) - }, context.RepoRef(), context.RequireRepoReaderOr(unit.TypeCode, unit.TypePullRequests, unit.TypeIssues, unit.TypeReleases)) - diff --git a/pkgs/forgejo/limit-migrations-and-pull-mirrors-to-admins.patch b/pkgs/forgejo/limit-migrations-and-pull-mirrors-to-admins.patch deleted file mode 100644 index 9221e69..0000000 --- a/pkgs/forgejo/limit-migrations-and-pull-mirrors-to-admins.patch +++ /dev/null @@ -1,53 +0,0 @@ -diff --git a/routers/api/v1/repo/migrate.go b/routers/api/v1/repo/migrate.go -index 2caaa130e8..455e89e93e 100644 ---- a/routers/api/v1/repo/migrate.go -+++ b/routers/api/v1/repo/migrate.go -@@ -12,7 +12,6 @@ import ( - - "code.gitea.io/gitea/models" - "code.gitea.io/gitea/models/db" -- "code.gitea.io/gitea/models/organization" - "code.gitea.io/gitea/models/perm" - access_model "code.gitea.io/gitea/models/perm/access" - repo_model "code.gitea.io/gitea/models/repo" -@@ -86,22 +85,7 @@ func Migrate(ctx *context.APIContext) { - } - - if !ctx.Doer.IsAdmin { -- if !repoOwner.IsOrganization() && ctx.Doer.ID != repoOwner.ID { -- ctx.Error(http.StatusForbidden, "", "Given user is not an organization.") -- return -- } -- -- if repoOwner.IsOrganization() { -- // Check ownership of organization. -- isOwner, err := organization.OrgFromUser(repoOwner).IsOwnedBy(ctx, ctx.Doer.ID) -- if err != nil { -- ctx.Error(http.StatusInternalServerError, "IsOwnedBy", err) -- return -- } else if !isOwner { -- ctx.Error(http.StatusForbidden, "", "Given user is not owner of organization.") -- return -- } -- } -+ ctx.Error(http.StatusForbidden, "", "You need to be administrator of this Forgejo instance to be able to create mirrors.") - } - - remoteAddr, err := forms.ParseRemoteAddr(form.CloneAddr, form.AuthUsername, form.AuthPassword) -diff --git a/routers/web/repo/migrate.go b/routers/web/repo/migrate.go -index 97b0c425ea..554a470eab 100644 ---- a/routers/web/repo/migrate.go -+++ b/routers/web/repo/migrate.go -@@ -150,6 +150,12 @@ func handleMigrateRemoteAddrError(ctx *context.Context, err error, tpl base.TplN - // MigratePost response for migrating from external git repository - func MigratePost(ctx *context.Context) { - form := web.GetForm(ctx).(*forms.MigrateRepoForm) -+ -+ if !ctx.Doer.IsAdmin { -+ ctx.Error(http.StatusForbidden, "MigratePost: you need to be site administrator to use migrations and mirrors") -+ return -+ } -+ - if setting.Repository.DisableMigrations { - ctx.Error(http.StatusForbidden, "MigratePost: the site administrator has disabled migrations") - return diff --git a/services/forgejo/default.nix b/services/forgejo/default.nix index a756e43..e2512c0 100644 --- a/services/forgejo/default.nix +++ b/services/forgejo/default.nix @@ -1,9 +1,10 @@ -{ pkgs, lib, config, ... }: +{ pkgs, lib, config, inputs, ... }: let cfg = config.bagel.services.forgejo; inherit (lib) mkIf mkEnableOption mkOption types; + nix-forgejo = import inputs.nix-forgejo { inherit pkgs; }; domain = "git.forkos.org"; in @@ -19,7 +20,7 @@ in services.forgejo = { enable = true; - package = pkgs.callPackage ../../pkgs/forgejo { }; + package = nix-forgejo.packages.forgejo; database = { type = "postgres";