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

Check if NIX_LINK_NEW exists instead of checking that NIX_LINK doesn't exist #7925

Merged
merged 2 commits into from
Mar 1, 2023

Conversation

cole-h
Copy link
Member

@cole-h cole-h commented Mar 1, 2023

Motivation

For brand new installations, neither NIX_LINK_NEW
($XDG_STATE_HOME/nix/profile or ~/.local/state/nix/profile), nor NIX_LINK (~/.nix-profile) will exist.

This restores functionality to nix-env, which is relied upon by GitHub Actions such as https://github.com/cachix/cachix-action and the Nixpkgs EditorConfig (and other) CI.

Context

Earlier today, Nixpkgs GHA CI started failing because it was relying on nix-env. Although this has since been fixed by switching to nix-shell, there are other places in the ecosystem that rely on 1) the latest Nix version; and 2) nix-env.

See the conversation in #5588 (starting at #5588 (comment)).

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

This might not be the right solution, but it is a solution that fixes the regression. I'm happy to close this in favor of a more comprehensive solution, however.

There's an extra commit pinning the cachix actions to Nix 2.13.3 to see if that will allow CI to pass (it's failing weirdly on master and prior to this change) -- feel free to force-push it away if this overall approach is "good enough" for now.

This should be backported to 2.14.

…t exist

For brand new installations, neither NIX_LINK_NEW
(`$XDG_STATE_HOME/nix/profile` or `~/.local/state/nix/profile`), nor
NIX_LINK (`~/.nix-profile`) will exist.

This restores functionality to nix-env, which is relied upon by GitHub
Actions such as https://github.com/cachix/cachix-action and the Nixpkgs
EditorConfig (and other) CI.
@cole-h cole-h requested a review from edolstra as a code owner March 1, 2023 00:21
@cole-h
Copy link
Member Author

cole-h commented Mar 1, 2023

I tested this in an Ubuntu 22.04.1 VM using the following (from a now-force-pushed-away commit, but the changes are ~the same):

set -eux
export XDG_STATE_HOME=
curl -L https://cole-h-nix-install-tests.cachix.org/serve/6k97myldi0hn3jq36m323cynafdnsb7z/install > install
chmod +x install
cat /dev/null | ./install --tarball-url-prefix https://cole-h-nix-install-tests.cachix.org/serve --daemon

. /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh

nix-env \
-iA editorconfig-checker \
-f '<nixpkgs>' \
-I nixpkgs=flake:nixpkgs \
--extra-experimental-features flakes

echo does-not-exist | xargs -r editorconfig-checker || echo FAILED!!!!!!!!! # if the file doesn't exist, it will print red text but not hit this "FAILED!!!" print; only if the command can't be found will it hit this print
nix-env -e editorconfig-checker

@cole-h cole-h mentioned this pull request Mar 1, 2023
8 tasks
@fricklerhandwerk fricklerhandwerk added the regression Something doesn't work anymore label Mar 1, 2023
@fricklerhandwerk
Copy link
Contributor

@balsoft @thufschmitt

Copy link
Member

@balsoft balsoft left a comment

Choose a reason for hiding this comment

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

I'm generally in favor of a fix, but I think there's a better solution. I'll try working on it separately

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/nix-2-14-0-released/25900/7

@balsoft
Copy link
Member

balsoft commented Mar 1, 2023

My solution is here: #7929

It's probably not perfect, but should be more correct in theory.

I'm still in favor of merging this PR first and rolling out the hotfix, since my solution will probably take longer to review and merge.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

Agreed with @balsoft's plan to move this first. Beyond the reasons given, when we make this feature non-experimental but still don't to immediately migrate all existing users, this will be the logic we want.

@github-actions
Copy link

github-actions bot commented Mar 1, 2023

@cole-h cole-h deleted the fixup-xdg-nix-env branch March 1, 2023 22:02
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

Git push to origin failed for 2.14-maintenance with exitcode 1

@cole-h
Copy link
Member Author

cole-h commented Mar 1, 2023

(FYI, this still included the commit pinning the install-nix-action to 2.13.3 -- I don't know if we want to keep that around, especially since v20 of the action fixes the incompatibility with 2.14.0)

PetarKirov added a commit to PetarKirov/dotfiles that referenced this pull request Mar 5, 2023
PetarKirov added a commit to PetarKirov/dotfiles that referenced this pull request Mar 5, 2023
PetarKirov added a commit to PetarKirov/dotfiles that referenced this pull request Mar 5, 2023
PetarKirov added a commit to metacraft-labs/nix-blockchain-development that referenced this pull request Apr 13, 2023
PetarKirov added a commit to metacraft-labs/nix-blockchain-development that referenced this pull request Apr 13, 2023
@balsoft balsoft mentioned this pull request May 23, 2023
12 tasks
@roberth
Copy link
Member

roberth commented Dec 11, 2023

This was merged without a test case. I hope that's not a pattern.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Something doesn't work anymore
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

8 participants