Skip to content

git: packaging cleanup#432010

Merged
philiptaron merged 3 commits intoNixOS:stagingfrom
tweag:balsoft/git-fixes
Aug 10, 2025
Merged

git: packaging cleanup#432010
philiptaron merged 3 commits intoNixOS:stagingfrom
tweag:balsoft/git-fixes

Conversation

@balsoft
Copy link
Member

@balsoft balsoft commented Aug 8, 2025

Shave some yaks in the git package. Mostly removes outdated workarounds and adds some comments.

  • git: re-enable "format" hardening
  • git: re-enable a test and remove unneded patch
  • git: add comments for patches

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.

@nixpkgs-ci nixpkgs-ci bot added 2.status: merge conflict This PR has merge conflicts with the target branch 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.approvals: 1 This PR was reviewed and approved by one person. labels Aug 8, 2025
@nix-owners nix-owners bot requested review from globin, kashw2 and wmertens August 8, 2025 13:39
@balsoft
Copy link
Member Author

balsoft commented Aug 8, 2025

Just realized that this belongs in staging, not master. Will fix.

@balsoft balsoft force-pushed the balsoft/git-fixes branch from 549eeeb to daea3df Compare August 8, 2025 15:04
@balsoft
git: re-enable "format" hardening
a959b9c 
Remove hardeningDisable = ["format"] because it is not necessary anymore
(the package builds without disabling it).
@balsoft balsoft force-pushed the balsoft/git-fixes branch from daea3df to 821b458 Compare August 8, 2025 15:06
@balsoft balsoft changed the base branch from master to staging August 8, 2025 15:06
@nixpkgs-ci nixpkgs-ci bot closed this Aug 8, 2025
@nixpkgs-ci nixpkgs-ci bot reopened this Aug 8, 2025
@balsoft balsoft force-pushed the balsoft/git-fixes branch from 821b458 to 2018c15 Compare August 8, 2025 15:12
@nixpkgs-ci nixpkgs-ci bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Aug 8, 2025
@nix-owners nix-owners bot requested review from me-and and philiptaron August 8, 2025 15:21
@philiptaron
Copy link
Contributor

philiptaron commented Aug 8, 2025

I won't have a chance to build and test this personally, but I'd be very happy to merge if Adam @me-and gets such a chance.

@balsoft
Copy link
Member Author

balsoft commented Aug 8, 2025

I'm in the process of nixpkgs-review on our build server, hang tight :)

@balsoft
Copy link
Member Author

balsoft commented Aug 8, 2025

It ran out of space :( I don't think I can rebuild all the dependencies myself. However the changes should only affect the installCheck phase of the git drv itself.

@me-and
Copy link
Member

me-and commented Aug 8, 2025

Yeah, nixpkgs-review will try to rebuild a lot of packages, for the same reason changes to Git get merged into staging rather than master. If you started your branch off staging, you'll also need to rebuild a lot of packages that would normally be downloaded from Hydra's builds, but the changes being in staging mean Hydra hasn't yet built them.

me-and
me-and approved these changes Aug 8, 2025
View reviewed changes
Copy link
Member

@me-and me-and left a comment

Choose a reason for hiding this comment

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

Changes look great, thank you for this tidy-up!

Only the slightest nit: you have a typo in one of your commit messages, where "unneded" should be "unneeded".

I'm running a more complete stack of tests overnight, but git.passthru.tests.withInstallCheck and all the fetchgit tests have passed, so I think it's very unlikely the other git* packages or the buildbot-integration tests will find any other problems.

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Aug 8, 2025
@balsoft
git: re-enable a test and remove unneeded patch
d10d07d 
Re-enables a test that now passes, and removes an unneeded patch that
was useful for testing at some point.
@balsoft
git: add comments for patches
3f2212c 
Adds inline comments for all patches as per CONTRIBUTING.md
@balsoft balsoft force-pushed the balsoft/git-fixes branch from 2018c15 to 3f2212c Compare August 9, 2025 08:30
@me-and
Copy link
Member

me-and commented Aug 9, 2025

Hmm. nixosTests.buildbot is failing to complete with these fixes, but is building successfully without the changes. The problem seems to be building python3.13-cfn-lint-1.32.1, but I can't see why these changes would affect that...

@me-and
Copy link
Member

me-and commented Aug 9, 2025

Ah, I think the cfn-lint problem is just that the package has a dependency on the system date. I've raised that as #432354, and I'm mildly annoyed at myself for taking this long to work out what was going on.

Anyway, with that explained, this LGTM!

@philiptaron philiptaron merged commit 6191a4b into NixOS:staging Aug 10, 2025
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@drupol drupol drupol approved these changes

@kashw2 kashw2 Awaiting requested review from kashw2

@wmertens wmertens Awaiting requested review from wmertens

@globin globin Awaiting requested review from globin

@philiptaron philiptaron Awaiting requested review from philiptaron

+1 more reviewer

@me-and me-and me-and approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

10.rebuild-darwin: 501+ This PR causes many rebuilds on Darwin and should normally target the staging branches. 10.rebuild-darwin: 5001+ This PR causes many rebuilds on Darwin and must target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 5001+ This PR causes many rebuilds on Linux and must target the staging branches. 12.approvals: 2 This PR was reviewed and approved by two 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.

4 participants

@balsoft @philiptaron @me-and @drupol