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

lvm2: put the profile scripts into etc/profile.d #373288

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from

Conversation

7c6f434c
Copy link
Member

Motivation: #369732

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:
    • NixOS test(s) (look inside [nixos/tests](https:// github.com/NixOS/nixpkgs/blob/master/nixos/tests))
    • and/or package tests
    • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
    • made sure NixOS tests are linked to the relevant packages
  • 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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: haskell 6.topic: qt/kde 6.topic: kernel The Linux kernel 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: emacs Text editor 6.topic: rust 6.topic: golang 6.topic: vim 6.topic: erlang 6.topic: ocaml 6.topic: fetch 6.topic: stdenv Standard environment 6.topic: nodejs 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: systemd 6.topic: julia 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 6.topic: llvm/clang Issues related to llvmPackages, clangStdenv and related 6.topic: dotnet Language: .NET 6.topic: flutter 6.topic: nvidia labels Jan 12, 2025
@nix-owners
Copy link

nix-owners bot commented Jan 12, 2025

The PR's base branch is set to master, but 1149 commits from the staging branch are included. Make sure you know the right base branch for your changes, then:

  • If the changes should go to the staging branch, change the base branch to staging
  • If the changes should go to the master branch, rebase your PR onto the merge base with the master branch:
    # git rebase --onto $(git merge-base upstream/master HEAD) $(git merge-base upstream/staging HEAD)
    git rebase --onto 93dbaf6d1a0385b5a7d9b378db1176b76db622fd e290d236884088ae2f61432eb72b5229ebf0404d
    git push --force-with-lease

@7c6f434c 7c6f434c changed the base branch from master to staging January 12, 2025 21:30
@github-actions github-actions bot removed 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: haskell 6.topic: qt/kde labels Jan 12, 2025
@wolfgangwalther
Copy link
Contributor

You still seem to have 11 commits from another branch in this PR. I suggest you rebase interactively and drop them - those are probably commits that were on master already, but didn't make their way to staging at the time, yet.

"--with-default-run-dir=/run/lvm"
"--with-systemdsystemunitdir=${placeholder "out"}/lib/systemd/system"
"--with-systemd-run=/run/current-system/systemd/bin/systemd-run"
"--with-default-profile-subdir=profile.d"
Copy link
Member Author

Choose a reason for hiding this comment

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

(this line is the added one)

Copy link
Contributor

Choose a reason for hiding this comment

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

If there's only a single line change, then I think you still need another rebase, so that the diff is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or did you run nixfmt on this file?

Then it would be good to split this into a separate commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

The latter, because otherwise CI complained, while the real change is hopefully pretty trivial.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should really split the nixfmt change and the actual change into separate commits. That would make it much easier to review. Even with whitespace changes invisible, it's a few changes...

@Ma27 Ma27 removed their request for review January 13, 2025 11:13
@7c6f434c
Copy link
Member Author

It looks like profile is never mentioned in the same file as lvm2 in the context of etc/profile path (but I am trying to run the tests just in case)

@wolfgangwalther wolfgangwalther removed their request for review January 14, 2025 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants