Improve error on conflict for nix profile install

[bobvanderlinden]

Motivation

Currently nix profile install gives a confusing and unhelpful error message whenever a package is installed that has conflicting files. This often happens when installing a different version of the same package.

This PR is an attempt to improve the experience in such situations to some extend. I see this as a small improvement, but it doesn't really reach the end-goal of a good UX yet.

As an example, this is what happens before this PR:

$ nix profile install github:cachix/devenv/v0.5
$ nix profile install github:cachix/devenv/v0.4
error: files '/nix/store/k0gwhiwfs8c218kph78vx75bph51iw31-devenv/bin/devenv' and '/nix/store/7i4hxhw44lr7r64zlmp4zq7cqpsx7smw-devenv/bin/devenv' have the same priority 5; use 'nix-env --set-flag priority NUMBER INSTALLED_PKGNAME' or type 'nix profile install --help' if using 'nix profile' to find out how to change the priority of one of the conflicting packages (0 being the highest priority)

This is what happens after this PR:

$ nix profile install github:cachix/devenv/v0.5
$ nix profile install github:cachix/devenv/v0.4
error: There is an existing package that has a conflicting file:

         /nix/store/7i4hxhw44lr7r64zlmp4zq7cqpsx7smw-devenv/bin/devenv

       The conflicting file:

         /nix/store/k0gwhiwfs8c218kph78vx75bph51iw31-devenv/bin/devenv

       You can remove the existing package using:

         nix profile remove github:cachix/devenv/v0.4

       You can also install the new package next to the existing package by assigning a priority to the package.
       The conflicting packages have a priority of 5.
       You can prioritize the new package using:

         nix profile install github:cachix/devenv/v0.4 --priority 4

       You can prioritize the existing package using:

         nix profile install github:cachix/devenv/v0.4 --priority 6

The new changes allows nix profile install to give more context.

This also avoids mentions of nix-env and nix profile when using builtins.buildEnv. Since buildEnv can be used in other places (unrelated to the Nix profile), it makes sense to avoid mentioning nix-env and nix profile, as they likely do not apply in these cases.

Context

Related to #7530

See my suggestion for a good UX on nix profiles. The current PR is merely an improvement for the error message on conflicts.

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

[domenkozar]

This looks great to me in terms of the error message improvement, but I'll let those that know the codebase comment on the C++ bits.

Thanks for improving this, it's quite a frustrating experience currently for devenv users to upgrade :)

[bobvanderlinden]

I made some more changes so that the INSTALLABLE of the suggested nix profile install commands can be filled in. The implementation first throws an error containing the conflicting files. Then the package path (/nix/store/XXXX-package) is looked up for the conflicting files. Lastly the INSTALLABLE from the manifest is found using the package path. All of this information is used in the resulting error.

[nixos-discourse]

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

https://discourse.nixos.org/t/contributing-to-nix/25605/1

[Ericson2314]

@bobvanderlinden fair warning I don't yet have perms to merge this, but I can still shepherd along making it an easier approval for those that do.

[Ericson2314]

@bobvanderlinden This is starting to look quite good! Thanks for doing this!

Correct me if I am wrong, but BuildEnvFileConflictError is just used in a private-to-the-file static function, so you should be able to not expose it in the header, but just have it in buildenv.cc.

Since it is for internal use only, and will always be caught, I think it would be also fine to just make it inherit std::exception rather than Error, and then you don't have to write a nice message which would be dead code.

[bobvanderlinden]

@bobvanderlinden This is starting to look quite good! Thanks for doing this!

Correct me if I am wrong, but BuildEnvFileConflictError is just used in a private-to-the-file static function, so you should be able to not expose it in the header, but just have it in buildenv.cc.
Since it is for internal use only, and will always be caught, I think it would be also fine to just make it inherit std::exception rather than Error, and then you don't have to write a nice message which would be dead code.

Ah you're right! That makes a lot of sense, thanks 👍 Updated the PR.

[Ericson2314]

Thanks @bobvanderlinden, this is very close to being ready!

[bobvanderlinden]

Thanks @bobvanderlinden, this is very close to being ready!

Thanks for the quick reviews and suggestions. This improves my long forgotten C++ knowledge a bit too. 🥳

[fricklerhandwerk]

In really like the informative error message, we can use lots more of that! Only suggestions on more concise wording from my side. Great work.

[bobvanderlinden]

The test did show that I made a mistake with the nix profile remove command where the conflicting installable was mixed up with the original installable, which I have now fixed. Good idea to add this test. Glad I did.

Thanks you all for the suggestions. I've incorporated most of them and resolved the discussions. Quite a few improvements to the PR already 👍

ideally we'd make this a markdown message and run a formatter on them for better presentation

Is this something I can look into for this PR?

[roberth]

ideally we'd make this a markdown message and run a formatter on them for better presentation

Is this something I can look into for this PR?

Better to do that in a followup, although it may make the testing story worse, as the layout becomes dependent on the length of the inputs.

[bobvanderlinden]

I have reworked the implementation in this PR and left the tests as-is.

I have removed BuildProfileConflictError and opted to merely throw BuildEnvFileConflictError. BuildEnvFileConflictError now holds fileA and fileB, which represent the 2 files that were conflicting inside buildEnv. Since we do not know at that time whether the files were from the original package or the newly installed package, I have called them A and B.

Determining which file belongs to the original/new package is done entirely in CmdProfileInstall::run. Packages that are being installed are placed at the end of manifest.elements, so from this I look up any package that matches one of the conflicting file paths at the end of manifest.elements to find which the new package and its conflicting file. I do the same thing for the already installed package, but by looking at the beginning of manifest.elements.

This gives an implementation that doesn't run into sorting issues that I can into before. Bonus: the code is cleaner this way as well.

[bobvanderlinden]

Hmm, MacOS in CI failed to build due to usage of C++20 ranges. I guess a different version of clang is used on MacOS to build Nix?
I now avoided using ranges and replaced it with begin() and rbegin() iterators. I tried compiling using clang and using nix develop .#nix-gccStdenv as well. 🤞

EDIT: The test failed again, this time because Nix in CI outputs a warning for running in offline mode, where-as locally that did not happen. I have opted for ignoring all warning: lines in the test.

[fricklerhandwerk]

@Ericson2314 I'll unsubscribe since the user-facing part is good, please continue with shepherding the implementation.

[Ericson2314]

Implementation looks good to me!

[fricklerhandwerk]

Merging it then.

[bobvanderlinden]

Thank you all 🥳👍

[nixos-discourse]

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

https://discourse.nixos.org/t/nix-team-report-2022-10-2023-03/27486/1