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.

Sign up for GitHub

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

Jump to bottom

python: Move catch_conflicts.py into subdirectory #23600

Merged
merged 1 commit into from Apr 1, 2017
Merged

python: Move catch_conflicts.py into subdirectory #23600

merged 1 commit into from Apr 1, 2017

Conversation

johbo
Copy link
Contributor

@johbo johbo commented Mar 7, 2017

Motivation for this change

Python does add the script's directory into "sys.path". For the case of
"catch_conflicts.py" this means "/nix/store" is added to "sys.path". This can
result in very long delays if the store contains a lot of entries.

I came across the issue on my machine which has about 100,000 entries in /nix/store. The build time for a single python derivation was increased by about 20 seconds due to this problem.

https://docs.python.org/3/library/sys.html#sys.path contains some details that the path of the script is added as first element into sys.path always.

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
    • Linux
  • 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.

@FRidh
Copy link
Member

FRidh commented Mar 8, 2017

Thanks for checking this out. This is an interesting issue.

Another solution would be to copy the file into the build as is done with run_setup.py in build-python-package-setuptools.nix. In any case, whatever solution you pick, please explain in the code why its done like that since otherwise someone would likely change it again.

@FRidh FRidh self-assigned this Mar 8, 2017
@FRidh FRidh added this to the 17.03 milestone Mar 8, 2017
@johbo
Copy link
Contributor Author

johbo commented Mar 8, 2017

Yes, indeed, we could copy it in as well, still it would need a subdirectory, otherwise we get false positives. Out of this thought I came to the conclusion to run this in an empty directory in the store. That's IMO less fragile.

Regarding a comment: Since it's a tricky issue, maybe it's best to put a small README.md into the subdirectory? Then I could explain the reasoning better and if someone wonders what this weird construction is about there is a good chance that it helps to understand the original reasons.

@FRidh
Copy link
Member

FRidh commented Mar 8, 2017

Yes, indeed, we could copy it in as well, still it would need a subdirectory, otherwise we get false positives. Out of this thought I came to the conclusion to run this in an empty directory in the store. That's IMO less fragile.

If it gets copied in, it would add the build directory to sys.path. What kind of false positives are you thinking of? In any case, you're right, having it in a separate directory is the safest.

Regarding a comment: Since it's a tricky issue, maybe it's best to put a small README.md into the subdirectory? Then I could explain the reasoning better and if someone wonders what this weird construction is about there is a good chance that it helps to understand the original reasons.

You could refer to this PR:

The file catch_conflicts.py is in a subdirectory because, if it isn't, the /nix/store/ directory is added to sys.path causing a delay when building. See #23600

@johbo
python: Move catch_conflicts.py into subdirectory
65b2491 
Python does add the script's directory into "sys.path". For the case of
"catch_conflicts.py" this means "/nix/store" is added to "sys.path". This can
result in very long delays if the store contains a lot of entries.
@dezgeg dezgeg mentioned this pull request Mar 8, 2017
Closed
@globin globin removed this from the 17.03 milestone Mar 14, 2017
@johbo
Copy link
Contributor Author

johbo commented Mar 15, 2017

Since this is flagged as a mass rebuild, does it need special treatment in any way?

@FRidh
Copy link
Member

FRidh commented Mar 18, 2017

@johbo mass-rebuilds go into staging.

Currently Hydra is quite occupied so I'll merge this together with some other Python-related mass-rebuilds (#22210).

@FRidh
Copy link
Member

FRidh commented Apr 1, 2017

@vcunat this can go in.

@vcunat vcunat merged commit 76213d1 into NixOS:master Apr 1, 2017
vcunat added a commit that referenced this pull request Apr 1, 2017
@vcunat
Revert "python: Move catch_conflicts.py into subdirectory (#23600)"
fc91ec7 
This reverts commit 76213d1.
@vcunat vcunat mentioned this pull request Apr 1, 2017
Merged
vcunat added a commit that referenced this pull request Apr 1, 2017
@vcunat
Revert "python: Move catch_conflicts.py into subdirectory (#23600)" (#…
46ea770 
…24528)

This reverts commit 76213d1.
vcunat pushed a commit that referenced this pull request Apr 1, 2017
@johbo @vcunat
python: Move catch_conflicts.py into subdirectory (#23600)
249b34a 
Python does add the script's directory into "sys.path". For the case of
"catch_conflicts.py" this means "/nix/store" is added to "sys.path". This can
result in very long delays if the store contains a lot of entries.

(moved from master commit 76213d1)
@vcunat
Copy link
Member

vcunat commented Apr 1, 2017

I didn't notice it was against master and had to "move" the commit to staging afterwards, creating a bit of mess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@FRidh FRidh

Labels
0.kind: enhancement 1.severity: mass-rebuild 6.topic: python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@johbo @FRidh @vcunat @globin