buildEnv: Check the content of colliding paths. #5096
Conversation
aszlig
added
the
9.needs: reporter feedback
aszlig
force-pushed
the
buildenv-check-collision-contents
branch
from
032a96b
to
7f5958c
Compare
|
+1. Nice feature. |
|
Doesn't enabling this by default break nixos-rebuild? nixos-rebuild by default ignores collisions, so checking the contents may break nixos-rebuild, am I wrong? |
|
This will also catch non-deterministic builds, something we can't do much about, right? So should there be a whitelist? |
|
@lethalman: It doesn't change anything in regards to nixos-rebuild, because whenever ignoreCollisions is active, this will still ignore collisions the same way. However, one particular thing could be done better: If ignoreCollisions is true, skip the content check entirely. Fixing... |
|
Do you have an example of a collision that would be prevented by this? |
|
@edolstra: This one for example: https://headcounter.org/hydra/build/583003/nixlog/1/raw |
|
Hm, never mind about the CUPS one, they do have different contents, actually. But I'd prefer to get out ignoreCollisions as much as possible, especially when it comes to the system path. So you only get collisions/warnings about files which actually have different content and might be a real problem. |
|
Is there any reason not to merge it? Removing ignoreCollisions from ghcWithPackages sounds like a good idea. |
|
It would also reduce the noise printed by |
|
Where exactly do we have collisions between binaries with the same name and identical contents? |
|
@edolstra: One thing that comes immediately into my mind would be reducing the noise while building the |
|
|
|
@aszlig: does this address the problem I see on IRC that a directory won't merge with a valid symlink to a directory? |
|
@vcunat: It doesn't handle directories at all, nor does it dereference symlinks, but the checkCollision could be extended to handle that as well. |
|
👍 I've had collisions between identical files quite often with texlive if I recall correctly, since texlive symlinks a lot of tools. |
|
Hehe, texlive won't be an issue any longer (also in this respect). I'll post updates to #287 when I get a little further (or request on IRC/mail if you hurry). |
|
What is the status of this? |
|
I'd like to merge this, @aszlig can you rebase? |
nbp
added
0.kind: enhancement
Originally wanted to include ignoreCollisions in cups-progs, but I think it's better if we use ignoreCollisions only if there are _real_ collisions between files with different contents. Of course, we also check whether the file permissions match, so you get a collision if contents are the same but the permissions are different. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
Checking file contents is redundant in this case, because we will go ahead anyway, regardless of whether the content is the same. Signed-off-by: aszlig <aszlig@redmoonstudios.org>
aszlig
force-pushed
the
buildenv-check-collision-contents
branch
from
b8db5af
to
bfb11fd
Compare
|
@domenkozar: Rebased. |
buildEnv: Check the content of colliding paths.
Regression introduced by 4529ed1. I've missed this in #5096, not because of a messed up rebase as I have guessed from a comment on #12635 but missed this in the first place. The testing I did while working on the pull request weren't exhaustive enough to cover this, because I haven't tested with packages that use the propagatedUserEnvPkgs attribute. In order to make the test a bit more exhaustive this time, let's test it using: nix-build -E 'with import ./. {}; buildEnv { name = "testenv"; paths = [ pkgs.hello pkgs.binutils pkgs.libsoup pkgs.gnome3.yelp pkgs.gnome3.totem ]; }' And with this commit the errors no longer show up and the environment is built correctly. Signed-off-by: aszlig <aszlig@redmoonstudios.org> Fixes: #12635
Originally wanted to include
ignoreCollisionsin cups-progs, but I think it's better if we useignoreCollisionsonly if there are real collisions between files with different contents.Of course, we also check whether the file permissions match, so you get a collision if contents are the same but the permissions are different.
Cc for review to @edolstra and @domenkozar