Skip to content

git: packaging cleanup #432010

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Aug 10, 2025
Merged

git: packaging cleanup #432010

merged 3 commits into from
Aug 10, 2025

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 kashw2, wmertens and globin 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 philiptaron and me-and August 8, 2025 15:21
@philiptaron
Copy link
Contributor

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