buildEnv: Don't warn about collisions if contents match when ignoreCollisions=true. #60365
Conversation
ambrop72
force-pushed
the
no-false-positive-collision-warnings
branch
from
8fc35c9
to
3a7c93e
Compare
|
I changed that line to |
…llisions=true. This prevents giving many false-positive collision warnings to NixOS users and makes it easier to find real collisions that should be addressed. Contents are checked only if checkCollisionContents is true, which is the case for the NixOS system-path by default. If checkCollisionContents is false, there is still a warning for every collision.
ambrop72
force-pushed
the
no-false-positive-collision-warnings
branch
from
3a7c93e
to
8734c75
Compare
With the current state of this PR, the behavior is very logical and consistent: The number of identical-content collisions in real NixOS configurations is so large that users are spammed with hundreds of collision errors, and real collisions are obscured (a different message doesn't help, once you get hundreds of warnings). So in the current state of NixOS, I can only see warnings about identical-content collisions as something that developers are interested in, not users. So if we have a feature to show this, I think it should be behind a flag and not the default (until the majority of identical-content collisions are resolved). |
That is indeed logical and consistent.
Collisions of identical files is not good either, but at least the implications aren't as bad. I agree that users preferably shouldn't see things that don't really impact them, but at the same time, hiding these type of collisions will also mean that the developers are also less likely to notice. Should we then have NixOS tests to test these don't occur? That obviously raises the question what packages to include. In my opinion a collision, regardless of whether the file is the same or not, is a bug, although I can imagine there are cases these are hard to prevent (Python 2 namespace packages e.g.). |
|
How about this: by default we don't warn for every identical-content collision, but if there were any, at the end we print a single message saying something like "Note: there were N collisions with identical file content, if you want to see them set environment.warnForIdenticalContentCollisions=true".
We could make buildEnv produce a secondary output that includes a file with collision information (e.g. a file with all collision messages) and tests could check that. This would be better than invoking |
|
Ok then I suggest we merge this, I anyway have more changes planned for buildEnv and I will implement more warning control soon. |
|
There are no objections, what is the hold-up? I will address the concern from @FRidh about letting one seeing all collisions in a follow-up PR, which will not be as simple as this one and will likely have some refactoring to the collision detection code to give better messages in general. |
|
Closing due to disagreements. I was planning to make other improvements to |
This prevents giving many false-positive collision warnings to NixOS users and makes it easier to see real collisions that should be addressed.
Looking at the history, @aszlig at one time committed a version that behaved like this but then immediately changed it to how it currently works, that is it warns for every collision without checking contents when ignoreCollision=true (as it is for the NixOS system path). While the resulting environment is indeed the same, I think it is worth doing the content check so that users are warned only about real collisions.
This reduced the number of collision warnings on my desktop system from 388 to 56.
Things done
I tested this with the NixOS system path in a hacky way so that it only affected that (the change as-is causes a mass rebuild because lots of things make use of buildEnv).
sandboxinnix.confon non-NixOS)nix-shell -p nix-review --run "nix-review wip"./result/bin/)nix path-info -Sbefore and after)