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

Global vs per-user profiles and channels #8147

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Apr 1, 2023

Motivation

The motivation for this is basically explained in the contribution guide, as guidance on how to design features like this going forward.

In short, I think it is confusing for operations acting on the default profile to always do something different based on whether the user is regular or root.

Instead, this makes there be flags which are (from the vantage point of today) "do the root default" vs "do the regular user" default. This make the situation teachable: we can point to the flags, and the conditional default, as exactly what varies between the root and non-root cases. And by manually specifying enough flags, we can ensure those defaults are overridden and Nix will indeed do the same thing (or fail trying).

Context

This is similar to the logic behind the supplementary group setting (#8342), which I also discuss in this new section of the contribution guide as a second example.

depends on #8351
depends on #8735

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

Priorities

Add 👍 to pull requests you find important.

@Ericson2314 Ericson2314 force-pushed the global-profile-flags branch from a1baec6 to 54123b0 Compare April 20, 2023 04:43
@Ericson2314 Ericson2314 force-pushed the global-profile-flags branch 2 times, most recently from 304acaa to adf8f48 Compare May 16, 2023 19:19
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 16, 2023
@dpulls
Copy link

dpulls bot commented Jun 14, 2023

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 force-pushed the global-profile-flags branch from adf8f48 to 0e1fe69 Compare July 12, 2023 17:53
@github-actions github-actions bot removed the with-tests Issues related to testing. PRs with tests have some priority label Jul 12, 2023
@Ericson2314 Ericson2314 force-pushed the global-profile-flags branch 3 times, most recently from 221c768 to d2f2c46 Compare July 24, 2023 19:19
@Ericson2314 Ericson2314 force-pushed the global-profile-flags branch from d2f2c46 to 7d1fc4d Compare August 2, 2023 17:02
@dpulls
Copy link

dpulls bot commented Aug 11, 2023

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 force-pushed the global-profile-flags branch from 7d1fc4d to 2c75e65 Compare August 11, 2023 18:58
@Ericson2314 Ericson2314 marked this pull request as ready for review August 11, 2023 19:02
@edolstra edolstra added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Aug 18, 2023
@Ericson2314
Copy link
Member Author

Ericson2314 commented Aug 18, 2023

Triaged in the Nix team meeting today

Idea approved, assigned to @tomberek


  • @Ericson2314: Heart of the motivation is the part in the contributors guide: no hard-coded "is root?" checks.

  • @edolstra: Idea seems good as long as there is no breaking changes, e.g. no having to configure something by hand that was automatic before

src/libstore/profiles.hh Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 force-pushed the global-profile-flags branch from 708694c to 963b079 Compare August 21, 2023 04:35
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-08-18-nix-team-meeting-minutes-80/32081/1

Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

Needs a bare-minimum of testing for the existence and sanity checking of the commands. Likely small modifications to the existing profile tests. This should not be a blocker - no need for exhaustiveness beyond existing test coverage.

eg: in user-envs.sh, running --global-profile seems to give the same results as --user-profile.

# Query installed: should contain foo-1.0 now (which should be
# executable).
test "$(nix-env -q '*' | wc -l)" -eq 1
test "$(nix-env --user-profile -q '*' | wc -l)" -eq 1
test "$(nix-env --global-profile -q '*' | wc -l)" -eq 0
nix-env -q '*' | grepQuiet foo-1.0
test "$($profiles/test/bin/foo)" = "foo-1.0"

@Ericson2314 Ericson2314 force-pushed the global-profile-flags branch 2 times, most recently from 427c5c3 to fd0a006 Compare August 25, 2023 14:31
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Aug 25, 2023
@Ericson2314 Ericson2314 marked this pull request as draft August 25, 2023 14:41
The motivation for this is basically explained in the contribution
guide, as guidance on how to design features like this going forward.

In short, I think it is confusing for operations acting on the default
profile to *always* do something different based on whether the user is
regular or root.

Instead, this makes there be flags which are (from the vantage point of
today) "do the root default" vs "do the regular user" default. This make
the situation teachable: we can point to the flags, and the conditional
default, as *exactly* what varies between the root and non-root cases.
And by manually specifying enough flags, we can ensure those defaults
are overridden and Nix will indeed do the same thing (or fail trying).

This is similar to the logic behind the supplementary group setting
(NixOS#8342), which I also discuss in this new section of the contribution
guide as a second example.

Instead of creating the default dirs in `getDefaultProfile` (which is a
bit odd if the symlink points elsewhere), create the profile dir for the
chosen profile in `createGeneration`. That seems more appropriate and
keeps the test added in e997512 (where
the profiles are deleted but the symlink isn't) working.
@Ericson2314 Ericson2314 force-pushed the global-profile-flags branch from 8ef788e to 6a378f5 Compare May 22, 2024 18:27
@github-actions github-actions bot added documentation contributor-experience Developer experience for Nix contributors labels May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors documentation idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

4 participants