Skip to content

nixos/etc: make sure local "source" files are imported to the store#136474

Merged
nrdxp merged 1 commit intoNixOS:masterfrom
waldheinz:etc-file-source-to-store
Sep 2, 2021
Merged

nixos/etc: make sure local "source" files are imported to the store#136474
nrdxp merged 1 commit intoNixOS:masterfrom
waldheinz:etc-file-source-to-store

Conversation

@waldheinz
Copy link
Contributor

Motivation for this change

The treatment of the "source" parameter changed
with eb7120d, breaking stuff.

Before that commit, the source parameter was converted to a
string by implicit coercion, which would copy the file to the
store and yield an string containing the store path. Now, by
the virtue of escapeShellArg, toString is called explicitly on
that path, which will yield an string containing the absolute
path of the file.

This commit restores the old behavior.

Things done
  • Built on platform(s)
    • x86_64-linux
  • Fits CONTRIBUTING.md.
  • Tested that the resulting /etc/ looks fine on NixOS

The treatment of the "source" parameter changed
with eb7120d, breaking stuff.

Before that commit, the source parameter was converted to a
string by implicit coercion, which would copy the file to the
store and yield an string containing the store path. Now, by
the virtue of escapeShellArg, toString is called explicitly on
that path, which will yield an string containing the absolute
path of the file.

This commit restores the old behavior.
@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 Sep 2, 2021
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 2, 2021
@Mic92 Mic92 requested a review from dasJ September 2, 2021 15:11
Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

untested but explanation and code makes sense to me.

@waldheinz
Copy link
Contributor Author

A somewhat extended explanation why I think this fix is the proper way to deal with the situation is also here:

NixOS/nix#5097 (comment)

Copy link

@nrdxp nrdxp left a comment

Choose a reason for hiding this comment

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

Tested your functions in a repl to ensure they produce the desired effect for my system config. Everything checks out. Good work!

Mic92 added a commit to Mic92/not-os that referenced this pull request Sep 3, 2021
This reverts commit ff6ea50.

No longer needed since NixOS/nixpkgs#136474
Mic92 added a commit to Mic92/not-os that referenced this pull request Sep 3, 2021
This reverts commit ff6ea50.

No longer needed since NixOS/nixpkgs#136474
@waldheinz waldheinz deleted the etc-file-source-to-store branch September 4, 2021 08:02
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 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants