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.

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/config: move nano to defaultPackages #97565

Merged
merged 1 commit into from Mar 31, 2021
Merged

nixos/config: move nano to defaultPackages #97565

merged 1 commit into from Mar 31, 2021

Conversation

samuelgrf
Copy link
Member

Motivation for this change

Some people already have another editor installed and may want to get rid of applications they don't use.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@samuelgrf
Copy link
Member Author

I just noticed typos in the option description, should I just amend these to this pr or open a new one?

diff --git a/nixos/modules/config/system-path.nix b/nixos/modules/config/system-path.nix
index e317a3a1ced..b366db2dfc9 100644
--- a/nixos/modules/config/system-path.nix
+++ b/nixos/modules/config/system-path.nix
@@ -74,7 +74,7 @@ in
         default = defaultPackages;
         example = literalExample "[]";
         description = ''
-          Set of packages users expect from a minimal linux istall.
+          Set of packages users expect from a minimal Linux install.
           Like systemPackages, they appear in
           /run/current-system/sw.  These packages are
           automatically available to all users, and are

@davidak
Copy link
Member

davidak commented Sep 9, 2020

Thanks!

I just noticed typos in the option description, should I just amend these to this pr or open a new one?

you can use this

davidak
davidak reviewed Sep 9, 2020
View reviewed changes
nixos/doc/manual/release-notes/rl-2009.xml Outdated
@@ -1050,7 +1050,7 @@ services.transmission.settings.rpc-bind-address = "0.0.0.0";
</listitem>
<listitem>
<para>
The option <option>defaultPackages</option> was added. It installs the packages <package>perl</package>, <package>rsync</package> and <package>strace</package> for now. They were added unconditionally to <option>systemPackages</option> before, but are not strictly necessary for a minimal NixOS install. You can set it to an empty list to have a more minimal system. Be aware that some functionality might still have an impure dependency on those packages, so things might break.
The option <option>defaultPackages</option> was added. It installs the packages <package>nano</package>, <package>perl</package>, <package>rsync</package> and <package>strace</package> for now. They were added unconditionally to <option>systemPackages</option> before, but are not strictly necessary for a minimal NixOS install. You can set it to an empty list to have a more minimal system. Be aware that some functionality might still have an impure dependency on those packages, so things might break.
Copy link
Member

Choose a reason for hiding this comment

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

this has to go in the changelog of 21.03 (which might not exist yet)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can't we just backport this pr? Nano is still installed by default, so this shouldn't affect any setups.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we could.

I want that defaultPackages can really be set to empty without causing any issues, so i would like more testing on unstable...

In this case it might be ok.

I have micro installed and use it most of the time, but git still opens nano. I haven't made the effort to change that yet. Have you tested for example such a case? And there might be edgecases like with rsync and nixops.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @davidak - I'm a big fan of removing packages causing side effects but let's keep this in unstable to bake for the next 6 months... we'll catch most issues that way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the thorough answers. I didn't think about the edge cases, so stabilizing this first makes sense, I'll update the pr.
Also, can someone proofread the release notes? English isn't my first language.

Copy link
Member

Choose a reason for hiding this comment

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

I'm also not a native speaker, but i see no issue. Thanks!

@samuelgrf samuelgrf force-pushed the defaultPackages-nano branch 2 times, most recently from 127e949 to 522a916 Compare September 15, 2020 18:37
@samuelgrf
Copy link
Member Author

In the last force-push I've rewritten the option description and added a note about installing another editor and setting the corresponding environment variable.
Git needs this for writing commit messages, else it throws an error.

davidak
davidak approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@davidak davidak left a comment

Choose a reason for hiding this comment

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

not tested, but looks good

@vikanezrimaya vikanezrimaya mentioned this pull request Oct 31, 2020
Open
@samuelgrf
nixos/config: move nano to defaultPackages
e6cd793 
Some people already have another editor installed and may want to
get rid of applications they don't use.
@samuelgrf
Copy link
Member Author

@davidak I've fixed the merge conflict and improved some of the wording, can you take another look and merge if it looks good?
I want this to get some testing on unstable, as we are getting close to 21.05's release.

davidak
davidak approved these changes Mar 31, 2021
View reviewed changes
Copy link
Member

@davidak davidak left a comment

Choose a reason for hiding this comment

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

Looks good!

This should not cause any issue. Maybe when someone set's the option to empty list, it could cause unexpected behavior.

If this causes bigger issues, it can get reverted before the release.

@davidak
Copy link
Member

davidak commented Mar 31, 2021

Result of nixpkgs-review pr 97565 1

2 packages blacklisted:
  • tests.nixos-functions.nixos-test
  • tests.nixos-functions.nixosTest-test

@davidak
Copy link
Member

davidak commented Mar 31, 2021

Result of nixpkgs-review pr 97565 1

1 test built:
  • nixosTests.env

@davidak davidak merged commit fe3eb35 into NixOS:master Mar 31, 2021
@das-g das-g mentioned this pull request Apr 20, 2021
Merged
10 tasks
@samuelgrf samuelgrf deleted the defaultPackages-nano branch March 5, 2022 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanderse aanderse aanderse left review comments

@davidak davidak davidak approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: changelog 8.has: documentation 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@samuelgrf @davidak @aanderse @ryantm