Skip to content

treewide: cleanup fetchFromGitLab default value#478670

Merged
SigmaSquadron merged 1 commit intoNixOS:masterfrom
qweered:cleanup-gitlab
Mar 2, 2026
Merged

treewide: cleanup fetchFromGitLab default value#478670
SigmaSquadron merged 1 commit intoNixOS:masterfrom
qweered:cleanup-gitlab

Conversation

@qweered
Copy link
Contributor

@qweered qweered commented Jan 10, 2026 •
edited
Loading

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@qweered qweered marked this pull request as draft January 10, 2026 08:45
@qweered qweered marked this pull request as ready for review January 10, 2026 09:00
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) labels Jan 10, 2026
@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Jan 12, 2026
@7c6f434c
Copy link
Member

7c6f434c commented Jan 12, 2026

What does this improve?

philiptaron
philiptaron requested changes Jan 13, 2026
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to leave things as they are. GitLab.com is just one instance of a GitLab server.

@qweered
Copy link
Contributor Author

qweered commented Jan 14, 2026 •
edited
Loading

we at least have domain = "gitlab.freedesktop.org"; and giving a helpful error message is nice when people use defaut value, it also reduces number of lines per package slightly. We don't have same problem for fetchFromGithub

dotlambda
dotlambda reviewed Jan 14, 2026
View reviewed changes
Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is called fetchFromGitLab, not fetchFromGitlab.

@qweered qweered changed the title treewide: cleanup fetchFromGitlab default value treewide: cleanup fetchFromGitLab default value Jan 14, 2026
@qweered
Copy link
Contributor Author

qweered commented Feb 10, 2026

cc @gepbird @SigmaSquadron for similar work on #483216

gepbird
gepbird reviewed Feb 10, 2026
View reviewed changes
Copy link
Contributor

@gepbird gepbird left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think removing domain = "gitlab.com" from packages is good for consistency with other fetchFromGitLab calls that don't have it set and it's -1 lines.

Given other people's comment on this, I'm not a fan of adding the assertion. It may give a few out of tree packages an unexpected failure and a few minutes of work/frustration when bumping nixpkgs with no real benefits other than consistency. Breaking changes due to bigger refactors that clean up tech debt is welcome, but this is not really it.

I'm not strongly against it, overall I think the small breakage outweighs the tiny benefits.

@qweered
Copy link
Contributor Author

qweered commented Feb 10, 2026 •
edited
Loading

I can convert assertion to warning, so it fails in nixpkgs ci but not in people config

SigmaSquadron
SigmaSquadron requested changes Feb 14, 2026
View reviewed changes
Copy link
Contributor

@SigmaSquadron SigmaSquadron left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole assertions (or warnings) system is pretty unnecessary. Just remove the duplicate code in the treewide and leave the builder as is.

@qweered
Copy link
Contributor Author

qweered commented Feb 18, 2026

Done

@nixpkgs-ci nixpkgs-ci bot removed the 6.topic: fetch Fetchers (e.g. fetchgit, fetchsvn, ...) label Feb 18, 2026
@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. 2.status: merge conflict This PR has merge conflicts with the target branch and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Feb 18, 2026
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Feb 19, 2026
@SigmaSquadron
Copy link
Contributor

SigmaSquadron commented Mar 2, 2026 •
edited
Loading

@philiptaron: I think the changes look good now, it's just removing duplicate code.

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Mar 2, 2026
philiptaron
philiptaron approved these changes Mar 2, 2026
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go with the team

@SigmaSquadron SigmaSquadron added this pull request to the merge queue Mar 2, 2026
Merged via the queue into NixOS:master with commit 2639078 Mar 2, 2026
31 of 33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dotlambda dotlambda dotlambda left review comments

@philiptaron philiptaron philiptaron approved these changes

@gepbird gepbird gepbird approved these changes

@SigmaSquadron SigmaSquadron SigmaSquadron approved these changes

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

@iblech iblech Awaiting requested review from iblech

@linsui linsui Awaiting requested review from linsui

@onny onny Awaiting requested review from onny

@OPNA2608 OPNA2608 Awaiting requested review from OPNA2608

@Pacman99 Pacman99 Awaiting requested review from Pacman99

@wegank wegank Awaiting requested review from wegank

+1 more reviewer

@nicknovitski nicknovitski nicknovitski approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@qweered @7c6f434c @SigmaSquadron @philiptaron @nicknovitski @dotlambda @gepbird