warn differently about collisions of identical paths #41144

jerith666 · 2018-05-27T17:16:14Z

Motivation for this change

As discussed at https://discourse.nixos.org/t/collisions-between-identical-files/278.

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

grahamc · 2018-05-27T17:23:12Z

pkgs/build-support/buildenv/builder.pl

@@ -110,7 +110,7 @@ sub findFiles {
    }

    unless (-d $target && ($oldTarget eq "" || -d $oldTarget)) {
-        if ($ignoreCollisions) {
+        if ($ignoreCollisions && !checkCollision($oldTarget, $target)) {


Should we also obey checkCollisionContents?

my reading of the code is that we have three possibilities:

ignore collisions with a warning

silently allow collisions, only if file contents are identical

don't allow any collisions at all

ignoreCollisions tells us if we're in case 1 or not. Then only if not, checkCollisionContents tells us if we're in 2 or 3.

Since all I'm aiming for is silencing meaningless warnings in case 1, I didn't think checkCollisionContents was relevant.

FRidh · 2018-05-29T19:48:09Z

We should aim at not having duplicate files. These warnings are needed in order to find them and therefore I'm of the opinion we should not silence them.

In your case (https://discourse.nixos.org/t/collisions-between-identical-files/278), an important question to ask is why you actually get multiple versions of Phonon in your profile. This is due to how we package Qt software; all Qt packages get propagated into the profile.

jerith666 · 2018-05-30T16:31:50Z

Hm ... would you be open to reporting collisions of identical files differently than collisions of different files? I think they're at least less severe than collisions of different files, right?

FRidh · 2018-05-30T16:58:56Z

Hm ... would you be open to reporting collisions of identical files differently than collisions of different files? I think they're at least less severe than collisions of different files, right?

It's additional information that could be useful, so if it's cheap to do, yes.

jerith666 · 2018-06-02T18:23:03Z

Okay, pushed a new version that still prints a message for identical collisions.

pkgs/build-support/buildenv/builder.pl

FRidh · 2018-06-02T18:28:28Z

Okay, pushed a new version that still prints a message for identical collisions.

Am I correct that collisions of identical files will now always be printed, and collisions of other files conditionally? That seems a bit weird.

jerith666 · 2018-06-02T18:36:25Z

Am I correct that collisions of identical files will now always be printed, and collisions of other files conditionally? That seems a bit weird.

I don't think so, but it's definitely worth talking through before biting off the mass-rebuild:

if we're in the $ignoreCollisions == true case (1st of the three clauses), then we'll always print something -- we just vary the message based on whether the paths are identical.

if we're in the $ignoreCollisions == false case (2nd and 3rd clauses), then we're only silent if the paths are identical (i.e. checkCollision() returns 1 / true). Otherwise, we die. (Well, actually, we unconditionally die if $checkCollisionContents == false.)

I think that's all okay ... ?

Mic92 · 2018-08-16T05:58:23Z

pkgs/build-support/buildenv/builder.pl

@@ -111,7 +111,11 @@ sub findFiles {

    unless (-d $target && ($oldTarget eq "" || -d $oldTarget)) {
        if ($ignoreCollisions) {
-            warn "collision between `$target' and `$oldTarget'\n" if $ignoreCollisions == 1;


What I also do not understand about the previous code is, why it checks first for if ($ignoreCollisions) and then has a second check if $ignoreCollisions == 1;. What other states can this variable has?

This one stumps me, too. I did a git grep ignoreCollisions, and every place it turned up was setting it to true or false -- in nix expressions. So ... hopefully those translate to 1 and 0 in perl, but I'm not really sure.

Found it:

commit 03f9026ac5ce7568c4b5c28d30552f869e229c63 Author: Eelco Dolstra <eelco.dolstra@logicblox.com> Date: Wed Jul 25 16:24:18 2012 -0400 buildEnv: don't warn about collisions in propagated packages This mimicks buildenv in Nix more closely. diff --git a/pkgs/build-support/buildenv/builder.pl b/pkgs/build-support/buildenv/builder.pl index 71502c9f668..08331b178f4 100755 --- a/pkgs/build-support/buildenv/builder.pl +++ b/pkgs/build-support/buildenv/builder.pl @@ -66,7 +66,7 @@ sub findFiles { unless (-d $target && ($oldTarget eq "" || -d $oldTarget)) { if ($ignoreCollisions) { - warn "collision between `$target' and `$oldTarget'"; + warn "collision between `$target' and `$oldTarget'" if $ignoreCollisions == 1; return; } else { die "collision between `$target' and `$oldTarget'"; @@ -122,7 +122,7 @@ while (scalar(keys %postponed) > 0) { my @pkgDirs = keys %postponed; %postponed = (); foreach my $pkgDir (sort @pkgDirs) { - addPkg($pkgDir, 1); + addPkg($pkgDir, 2); } }

The use of addPkg has changed since 2012, but there's still a case where $ignoreCollisions can be 2. It's on line 198 currently.

Therefore I think it makes sense to keep the if $ignoreCollisions == 1 in both cases

edit I mean that it should be refactored so == 1 is always checked, of course. :)

It would be great to have a comment clarifying this.

pkgs/build-support/buildenv/builder.pl

such collisions are still a concern, but not as severe as collisions between non-identical paths.

nixos-discourse · 2018-11-28T03:19:00Z

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/how-to-debug-a-conflict/1524/7

matthewbauer · 2019-03-09T01:45:52Z

Looks okay to me... Is there anything left here?

stale · 2020-06-02T02:48:13Z

Thank you for your contributions.
This has been automatically marked as stale because it has had no activity for 180 days.
If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.
Here are suggestions that might help resolve this more quickly:

Search for maintainers and people that previously touched the
related code and @ mention them in a comment.
Ask on the NixOS Discourse. 3. Ask on the #nixos channel on
irc.freenode.net.

Mic92 · 2020-06-02T10:39:36Z

@matthewbauer I think it only needs somebody to test/merge it.

nixos-discourse · 2021-01-05T23:00:25Z

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/431

stale · 2021-07-08T00:22:21Z

I marked this as stale due to inactivity. → More info

AndersonTorres · 2023-03-05T00:34:56Z

Closing as dead.

grahamc reviewed May 27, 2018

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ labels May 27, 2018

jerith666 force-pushed the identical-collisions branch from ca1085d to cef25e5 Compare June 2, 2018 18:22

jerith666 changed the title ~~silence warnings about collisions of identical files~~ warn differently about collisions of identical paths Jun 2, 2018

FRidh reviewed Jun 2, 2018

View reviewed changes

pkgs/build-support/buildenv/builder.pl Outdated Show resolved Hide resolved

jerith666 force-pushed the identical-collisions branch from cef25e5 to 635b608 Compare June 2, 2018 18:29

Mic92 reviewed Aug 16, 2018

View reviewed changes

pkgs/build-support/buildenv/builder.pl Outdated Show resolved Hide resolved

warning differently about collisions of identical paths

388a3b6

such collisions are still a concern, but not as severe as collisions between non-identical paths.

jerith666 force-pushed the identical-collisions branch from 635b608 to 388a3b6 Compare August 18, 2018 01:46

ambrop72 mentioned this pull request May 4, 2019

buildEnv: Don't warn about collisions if contents match when ignoreCollisions=true. #60365

Closed

10 tasks

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 2, 2020

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 8, 2021

AndersonTorres closed this Mar 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

warn differently about collisions of identical paths #41144

warn differently about collisions of identical paths #41144

jerith666 commented May 27, 2018

grahamc May 27, 2018

jerith666 May 27, 2018

FRidh commented May 29, 2018

jerith666 commented May 30, 2018

FRidh commented May 30, 2018

jerith666 commented Jun 2, 2018

FRidh commented Jun 2, 2018

jerith666 commented Jun 2, 2018

Mic92 Aug 16, 2018

jerith666 Aug 18, 2018

chreekat Mar 25, 2019

chreekat Mar 25, 2019 •

edited

samuela Sep 23, 2020

nixos-discourse commented Nov 28, 2018

matthewbauer commented Mar 9, 2019

stale bot commented Jun 2, 2020

Mic92 commented Jun 2, 2020 •

edited

nixos-discourse commented Jan 5, 2021

stale bot commented Jul 8, 2021

AndersonTorres commented Mar 5, 2023

warn differently about collisions of identical paths #41144

warn differently about collisions of identical paths #41144

Conversation

jerith666 commented May 27, 2018

Motivation for this change

Things done

grahamc May 27, 2018

Choose a reason for hiding this comment

jerith666 May 27, 2018

Choose a reason for hiding this comment

FRidh commented May 29, 2018

jerith666 commented May 30, 2018

FRidh commented May 30, 2018

jerith666 commented Jun 2, 2018

FRidh commented Jun 2, 2018

jerith666 commented Jun 2, 2018

Mic92 Aug 16, 2018

Choose a reason for hiding this comment

jerith666 Aug 18, 2018

Choose a reason for hiding this comment

chreekat Mar 25, 2019

Choose a reason for hiding this comment

chreekat Mar 25, 2019 • edited

Choose a reason for hiding this comment

samuela Sep 23, 2020

Choose a reason for hiding this comment

nixos-discourse commented Nov 28, 2018

matthewbauer commented Mar 9, 2019

stale bot commented Jun 2, 2020

Mic92 commented Jun 2, 2020 • edited

nixos-discourse commented Jan 5, 2021

stale bot commented Jul 8, 2021

AndersonTorres commented Mar 5, 2023

chreekat Mar 25, 2019 •

edited

Mic92 commented Jun 2, 2020 •

edited