Skip to content
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

nixos/installation-cd-base: add git & rsync #325511

Merged
merged 1 commit into from
Dec 1, 2024
Merged

nixos/installation-cd-base: add git & rsync #325511

merged 1 commit into from
Dec 1, 2024

Conversation

JerrySM64
Copy link
Member

Description of changes

Add git and rsync to installation-cd-base so the minimal iso also has those tools out of the box. Fixes #325170

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 8, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jul 8, 2024
Frontear
Frontear approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@Frontear Frontear left a comment

Choose a reason for hiding this comment

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

This would be a great change. Approved!

@Gerg-L
Copy link
Contributor

Gerg-L commented Jul 8, 2024

should gitMinimal be used instead?
it's a third the closure size

$ nix path-info -Sh $(nix build --print-out-paths nixpkgs#git)
/nix/store/1zyjd4qmdgwrkvcvmxddrka6lzzx1h2b-git-2.44.1   316.3 MiB
$ nix path-info -Sh $(nix build --print-out-paths nixpkgs#gitMinimal)
/nix/store/vjzkh497v13asja08g41zilix8lnfsnq-git-minimal-2.44.1   114.7 MiB

the only downside i can see is that it doesn't include manual pages

@Gerg-L
Copy link
Contributor

Gerg-L commented Jul 8, 2024 •
edited
Loading

Tested on nixos-unstable like so:

cd nixos
nix-build -A config.system.build.isoImage -I nixos-config=modules/installer/cd-dvd/installation-cd-minimal.nix default.nix
du -h $(readlink ./result)

baseline: 1052810

git+rysnc: 1072462
gitMinimal+rsync: 1068182
git only: 1072098
rsync only: 1014754

ok so git vs gitMinimal does not make a significant difference

I would also remove these lines in this PR

nixpkgs/nixos/modules/installer/cd-dvd/installation-cd-graphical-base.nix

Lines 59 to 61 in 67ab30a

# Include some version control tools.
git
rsync

other than that looks good

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jul 8, 2024
@JerrySM64
Copy link
Member Author

Will squash in a moment. The Web IDE doesn't let me do it.

NotAShelf
NotAShelf approved these changes Jul 9, 2024
View reviewed changes
Copy link
Member

@NotAShelf NotAShelf left a comment

Choose a reason for hiding this comment

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

LGTM

@JerrySM64
Copy link
Member Author

Do I still need to squash? That LGTM confused me a bit.

@NotAShelf
Copy link
Member

Yes you still need to squash, I didn't notice the history sorry.

@JerrySM64
Copy link
Member Author

Alright, it is now squashed. Took a bit, because git was acting up with the rebase command, sorry.

Gerg-L
Gerg-L approved these changes Jul 9, 2024
View reviewed changes
Copy link
Contributor

@Gerg-L Gerg-L left a comment

Choose a reason for hiding this comment

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

LGTM

Pandapip1
Pandapip1 approved these changes Jul 10, 2024
View reviewed changes
Copy link
Contributor

@Pandapip1 Pandapip1 left a comment

Choose a reason for hiding this comment

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

Diff lgtm

@wegank wegank added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Jul 10, 2024
@JerrySM64
Copy link
Member Author

Is it going to be merged or do we hold it for the other possible solutions mentioned in the issue I mentioned in the beginning? Not trying to be rude or impatient, just curious.

@Gerg-L
Copy link
Contributor

Gerg-L commented Jul 11, 2024

Will probably be merged whenever someone with merged access glaces at it

Gerg-L
Gerg-L approved these changes Jul 13, 2024
View reviewed changes
Copy link
Contributor

@Gerg-L Gerg-L left a comment

Choose a reason for hiding this comment

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

LGTM

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/4541

Maxito7
Maxito7 approved these changes Nov 8, 2024
View reviewed changes
Copy link

@Maxito7 Maxito7 left a comment

Choose a reason for hiding this comment

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

LGTM!

myclevorname
myclevorname approved these changes Nov 9, 2024
View reviewed changes
Copy link
Contributor

@myclevorname myclevorname left a comment

Choose a reason for hiding this comment

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

LGTM

@myclevorname
Copy link
Contributor

Will probably be merged whenever someone with merged access glaces at it

@Aleksanaa please

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/2102

@FliegendeWurst FliegendeWurst added the needs_merger (old Marvin label, do not use) label Nov 14, 2024
@misuzu misuzu merged commit 2a1bf49 into NixOS:master Dec 1, 2024
24 checks passed
@iynaix iynaix added the backport release-24.11 Backport PR automatically label Dec 2, 2024
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 2, 2024

Successfully created backport PR for release-24.11:

@github-actions github-actions bot mentioned this pull request Dec 2, 2024
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@myclevorname myclevorname myclevorname approved these changes

@Gerg-L Gerg-L Gerg-L approved these changes

@Maxito7 Maxito7 Maxito7 approved these changes

@LilleAila LilleAila LilleAila approved these changes

@NotAShelf NotAShelf NotAShelf approved these changes

@Pandapip1 Pandapip1 Pandapip1 approved these changes

@Frontear Frontear Frontear approved these changes

@FliegendeWurst FliegendeWurst FliegendeWurst approved these changes

@iynaix iynaix iynaix approved these changes

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people backport release-24.11 Backport PR automatically needs_merger (old Marvin label, do not use)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

minimal iso does not include git