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

home-manager: do not build news when using flake #2501

Merged
merged 1 commit into from Nov 25, 2021

Conversation

polykernel
Copy link
Contributor

@polykernel polykernel commented Nov 22, 2021

Currently, the buildNews and doBuildAttrs are always called
unconditionally even if a flake configuration is specified. This cause
it to always fail prior to the actual build performed by doBuildAttrs
because setConfigFIle can not find the home-manager configuration file.
As a result, an error message specifying no configuration file is shown.

Furthermore, if a user has remnant legacy configuration, the doSwitch and
doBuild functions will effectively build the activationPackage twice, with
the legacy configuration overriding the flake configuration.

A conditional check for FLAKE_CONFIG_URI was added to mitigate this by building
the legacy configuration when no flake configuration is present. There is one
exception which is when a flake configuration exists in the default location, where
the user can not build the legacy configuration as along as the file is present.
However, the tradeoff is acceptable as it matches current behavior when FLAKe_CONFIG_URI
is set for instantiation, and an user is unlikely to simulataneously switch
between the two mechanisms.

An abstract function for building flakes doBuildFlake was created to match
doBuildAttrs for manageing options and build flags.

The --no-write-lock-file flag was removed from the --debug case as it is already
matched previously at the --recreate-lock-file case.

Description

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

Currently, the `buildNews` and `doBuildAttrs` are always called
unconditionally even if a flake configuration is specified. This cause
it to always fail prior to the actual build performed by `doBuildAttrs`
because `setConfigFIle` can not find the home-manager configuration file.
As a result, an error message specifying no configuration file is shown.

Furthermore, if a user has remnant legacy configuration, the `doSwitch` and
`doBuild` functions will effectively build the activationPackage twice, with
the legacy configuration overriding the flake configuration.

A conditional check for FLAKE_CONFIG_URI was added to mitigate this by building
the legacy configuration when no flake configuration is present. There is one
exception which is when a flake configuration exists in the default location, where
the user can not build the legacy configuration as along as the file is present.
However, the tradeoff is acceptable as it matches current behavior when FLAKe_CONFIG_URI
is set for instantiation, and an user is unlikely to simulataneously switch
between the two mechanisms.

An abstract function for building flakes `doBuildFlake` was created to match
`doBuildAttrs` for  manageing options and build flags.

The --no-write-lock-file flag was removed from the --debug case as it is already
matched previously at the --recreate-lock-file case.
home-manager/home-manager Show resolved Hide resolved
Copy link
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

I don't have a standalone HM configuration ATM so I trust that it works for you 😄

@berbiche berbiche merged commit 6093706 into nix-community:master Nov 25, 2021
@polykernel polykernel deleted the home-manager-patch-1 branch November 25, 2021 00:46
@SuperSandro2000
Copy link
Member

Does this fix the following?

home-manager news
error: infinite recursion encountered

       at $HOME/src/nixpkgs/lib/modules.nix:365:28:

          364|         builtins.addErrorContext (context name)
          365|           (args.${name} or config._module.args.${name})
             |                            ^
          366|       ) (lib.functionArgs f);
(use '--show-trace' to show detailed location information)
$HOME/.nix-profile/bin/home-manager: line 425: /tmp/home-manager-build.M3nPK4D340/news-info.sh: No such file or directory

Also how do I now get news on flakes?

@polykernel
Copy link
Contributor Author

polykernel commented Nov 26, 2021

@SuperSandro2000 I don't think I changed anything relating to the presentNews and buildNews functions, I think the only thing that could have changed is news(which must be built using search paths) isn't built unconditionally if flake is used. The previous behavior was to build the flake activationPackage first, followed by legacy activationPackage then news. Unfortunately, this means it is currently impossible to build news with flakes, however I am working on integrating home-manager.nix with modules.nix somehow so it can be exposed in the root flake.

@SuperSandro2000
Copy link
Member

was to build the flake activationPackage first, followed by legacy activationPackage then news.

I might have seen that before already. Always wondered why it takes so much longer.

I am working on integrating home-manager.nix with modules.nix somehow so it can be exposed in the root flake.

If you remember please ping me for testing.

I think my above error was due to a bad overlay which is not obvious at all.

polykernel added a commit to polykernel/home-manager that referenced this pull request Nov 28, 2021
Two misplaced quotations were introduced in `doBuild` by nix-community#2501, which
caused the parameter expansion of DRY_RUN to include an extraneous tab. Since the flake uri is passed
later into the command, Nix assumes the whitespace sequence as the flake uri and returns that it is not
a valid flake reference.

This PR removes the misplaced quotations in `doBuild` and also places the flake uri as the first argument for
calls to `doBuildFlake` for consistency with `doBuildAttr`. Placing the uri first in the command line also guard
possible security issues if arbitrary uris are expanded prior to the user given uri.
polykernel added a commit to polykernel/home-manager that referenced this pull request Nov 28, 2021
Two misplaced quotations were introduced in `doBuild` by nix-community#2501, which
caused the parameter expansion of DRY_RUN to include an extraneous tab. Since the flake uri is passed
later into the command, Nix assumes the whitespace sequence as the flake uri and returns that it is not
a valid flake reference.

This PR removes the misplaced quotations in `doBuild` and also places the flake uri as the first argument for
calls to `doBuildFlake` for consistency with `doBuildAttr`. Placing the uri first in the command line also guards
against possible security issues if arbitrary uris are expanded prior to the user given uri.
berbiche pushed a commit that referenced this pull request Nov 29, 2021
Two misplaced quotations were introduced in `doBuild` by #2501, which
caused the parameter expansion of DRY_RUN to include an extraneous tab. Since the flake uri is passed
later into the command, Nix assumes the whitespace sequence as the flake uri and returns that it is not
a valid flake reference.

This PR removes the misplaced quotations in `doBuild` and also places the flake uri as the first argument for
calls to `doBuildFlake` for consistency with `doBuildAttr`. Placing the uri first in the command line also guards
against possible security issues if arbitrary uris are expanded prior to the user given uri.
peterhoeg pushed a commit to peterhoeg/home-manager that referenced this pull request Nov 29, 2021
Currently, the `buildNews` and `doBuildAttrs` are always called
unconditionally even if a flake configuration is specified. This cause
it to always fail prior to the actual build performed by `doBuildAttrs`
because `setConfigFIle` can not find the home-manager configuration file.
As a result, an error message specifying no configuration file is shown.

Furthermore, if a user has remnant legacy configuration, the `doSwitch` and
`doBuild` functions will effectively build the activationPackage twice, with
the legacy configuration overriding the flake configuration.

A conditional check for FLAKE_CONFIG_URI was added to mitigate this by building
the legacy configuration when no flake configuration is present. There is one
exception which is when a flake configuration exists in the default location, where
the user can not build the legacy configuration as along as the file is present.
However, the tradeoff is acceptable as it matches current behavior when FLAKe_CONFIG_URI
is set for instantiation, and an user is unlikely to simulataneously switch
between the two mechanisms.

An abstract function for building flakes `doBuildFlake` was created to match
`doBuildAttrs` for  manageing options and build flags.

The --no-write-lock-file flag was removed from the --debug case as it is already
matched previously at the --recreate-lock-file case.
rycee pushed a commit that referenced this pull request Dec 2, 2021
Two misplaced quotations were introduced in `doBuild` by #2501, which
caused the parameter expansion of DRY_RUN to include an extraneous tab. Since the flake uri is passed
later into the command, Nix assumes the whitespace sequence as the flake uri and returns that it is not
a valid flake reference.

This PR removes the misplaced quotations in `doBuild` and also places the flake uri as the first argument for
calls to `doBuildFlake` for consistency with `doBuildAttr`. Placing the uri first in the command line also guards
against possible security issues if arbitrary uris are expanded prior to the user given uri.

(cherry picked from commit 9de7722)
peterhoeg pushed a commit to peterhoeg/home-manager that referenced this pull request Dec 6, 2021
Two misplaced quotations were introduced in `doBuild` by nix-community#2501, which
caused the parameter expansion of DRY_RUN to include an extraneous tab. Since the flake uri is passed
later into the command, Nix assumes the whitespace sequence as the flake uri and returns that it is not
a valid flake reference.

This PR removes the misplaced quotations in `doBuild` and also places the flake uri as the first argument for
calls to `doBuildFlake` for consistency with `doBuildAttr`. Placing the uri first in the command line also guards
against possible security issues if arbitrary uris are expanded prior to the user given uri.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants