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

added dependency to systemd #199425

Merged
merged 1 commit into from
Jan 1, 2023
Merged

added dependency to systemd #199425

merged 1 commit into from
Jan 1, 2023

Conversation

Mikilio
Copy link
Contributor

@Mikilio Mikilio commented Nov 3, 2022

Description of changes

Fix for issue #39140

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@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 Nov 3, 2022
@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 Nov 3, 2022
@Mikilio Mikilio force-pushed the master branch 2 times, most recently from 26219f8 to e9315a2 Compare November 5, 2022 12:21
@chkno
Copy link
Member

chkno commented Nov 22, 2022

This bakes the path to systemd into nixos-enter at nixos-enter's build time. I'm worried that the path to system might change between nixos-enter's build time and its run time -- for example, if the user updates channels in the install environment.

Instead of changing PATH and finding systemd-tmpfiles with a PATH lookp, consider specifying the full path to systemd-tmpfiles as $system/sw/bin/systemd-tmpfiles . This follows the convention used elsewhere in nixos-enter, which includes the ability to set $system via the --system flag.

Also, it would be great to have a test that could have caught this. I appear to have somehow neglected to submit the tests linked in #39140 (comment) (oops!), but they would not have caught this problem because the nixos-installer has a PATH that will find a systemd-tmpfiles in the chroot. It's the users trying to install from environments like Kimsufi's rescue environment that were exposed to this error. But setting up a new installer test from a wierd but realistic environment is ~1000x more work than just making this fix, so don't let that block merge of this PR.

Thanks for fixing this!

@Mikilio
Copy link
Contributor Author

Mikilio commented Nov 22, 2022

This is my first pull request to this repo, so I'm sorry if I'm not familiar with common practices here:
Do you want me to revert my changes and substitute systemd-tmpfiles with a full path instead? Or has this already been done?

@chkno
Copy link
Member

chkno commented Nov 23, 2022

Fantastic! Welcome!

Concretely, I mean that this change would be a better way to fix this problem than the changes this PR is currently proposing.

Mechanically, I meant to suggest that you change this PR to propose that change instead, by force-pushing https://github.com/Mikilio/nixpkgs to be a single commit that makes that change. One way to do that would be to git reset --hard HEAD^, make that change, commit, and force-push.

Alternatively, if you prefer, I could propose that change as a new PR.

(I introduced this bug in #80769 . Thank you for identifying the problem and helping get it fixed.)

@Mikilio
Copy link
Contributor Author

Mikilio commented Nov 23, 2022

Ok! Since I agree with your points I'll change my PR. Thanks for the advice!

@chkno
Copy link
Member

chkno commented Nov 23, 2022

Great!

Next, among other things, that "Fits CONTRIBUTING.md." checkbox in the PR template is meant to encourage good, uniform commit messages. So please skim that, look at the commit messages of some other commits in that place, change your commit message to follow those conventions (concretely: such as with git commit --amend), and force push again.

And thanks for bearing with the get-to-know-the-project-conventions first-PR back-and-forth. :)

@Mikilio
Copy link
Contributor Author

Mikilio commented Nov 24, 2022

Thank you for the guidance!

@mweinelt mweinelt added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 3, 2022
Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

This change looks good so I'm going to merge now, but I'm curious why the chosen route was to make nixos-enter create its own /tmp, instead of simply fixing TMPDIR in nixos-install? By that point we've already created a temporary directory under the mountpoint, so TMPDIR=${TMPDIR#"$mountPoint"} nixos-enter ... should suffice?

Calling systemd-tmpfiles in the nixos-enter environment also has the annoying side-effect that it prints a bunch of red warnings like

/etc/tmpfiles.d/systemd.conf:23: Failed to replace specifiers in '/run/log/journal/%m': No such file or directory

because /etc/machine-id does not exist in the mounted root.

EDIT: I see now that creating /tmp in nixos-enter is needed for nix-build to work.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

Successfully created backport PR #208607 for release-22.05.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

Successfully created backport PR #208608 for release-22.11.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

Git push to origin failed for release-22.05 with exitcode 1

@github-actions
Copy link
Contributor

github-actions bot commented Jan 1, 2023

Git push to origin failed for release-22.11 with exitcode 1

@Janik-Haag Janik-Haag added the 12. first-time contribution This PR is the author's first one; please be gentle! label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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: 1 This PR was reviewed and approved by one reputable person 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants