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.

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

Python: introduce NIX_PYTHONPREFIX in order to set site.PREFIXES (20.03 backport) #86487

Merged
merged 2 commits into from May 2, 2020

Conversation

adisbladis
Copy link
Member

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@adisbladis adisbladis requested a review from FRidh as a code owner May 1, 2020 17:00
@FRidh
Copy link
Member

FRidh commented May 1, 2020

best send this to staging-20.03

This is needed in case of `python.buildEnv` to make sure site.PREFIXES
does not only point to the unwrapped executable prefix.

--------------------------------------------------------------------------------

This PR is a story where your valiant hero sets out on a very simple adventure but ends up having to slay dragons, starts questioning his own sanity and finally manages to gain enough knowledge to slay the evil dragon and finally win the proverbial price.

It all started out on sunny spring day with trying to tackle the Nixops plugin infrastructure and make that nice enough to work with.

Our story begins in the shanty town of [NixOps-AWS](https://github.com/nixos/nixops-aws) where [mypy](http://mypy-lang.org/) type checking has not yet been seen.

As our deuteragonist (@grahamc) has made great strides in the capital city of [NixOps](https://github.com/nixos/nixops) our hero wanted to bring this out into the land and let the people rejoice in reliability and a wonderful development experience.

The plugin work itself was straight forward and our hero quickly slayed the first small dragon, at this point things felt good and our hero thought he was going to reach the town of NixOps-AWS very quickly.

But alas! Mypy did not want to go, it said:
`Cannot find implementation or library stub for module named 'nixops'`

Our hero felt a small sliver of life escape from his body. Things were not going to be so easy.

After some frustration our hero discovered there was a [rule of the land of Python](https://www.python.org/dev/peps/pep-0561/) that governed the import of types into the kingdom, more specificaly a very special document (file) called `py.typed`.
Things were looking good.

But no, what the law said did not seem to match reality. How could things be so?

After some frustrating debugging our valiant hero thought to himself "Hmm, I wonder if this is simply a Nix idiosyncrasy", and it turns out indeed it was.
Things that were working in the blessed way of the land of Python (inside a `virtualenv`) were not working the way they were from his home town of Nix (`nix-shell` + `python.withPackages`).

After even more frustrating attempts at reading the mypy documentation and trying to understand how things were supposed to work our hero started questioning his sanity.
This is where things started to get truly interesting.

Our hero started to use a number of powerful weapons, both forged in the land of Python (pdb) & by the mages of UNIX (printf-style-debugging & strace).

After first trying to slay the dragon simply by `strace` and a keen eye our hero did not spot any weak points.
Time to break out a more powerful sword (`pdb`) which also did not divulge any secrets about what was wrong.

Our hero went back to the `strace` output and after a fair bit of thought and analysis a pattern started to emerge. Mypy was looking in the wrong place (i.e. not in in the environment created by `python.withPackages` but in the interpreter store path) and our princess was in another castle!

Our hero went to the pub full of old grumpy men giving out the inner workings of the open source universe (Github) and acquired a copy of Mypy.
He littered the code with print statements & break points.
After a fierce battle full of blood, sweat & tears he ended up in https://github.com/python/mypy/blob/20f7f2dd71c21bde4d3d99f9ab69bf6670c7fa03/mypy/sitepkgs.py and realised that everything came down to the Python `site` module and more specifically https://docs.python.org/3.7/library/site.html#site.getsitepackages which in turn relies on https://docs.python.org/3.7/library/site.html#site.PREFIXES .

Our hero created a copy of the environment created by `python.withPackages` and manually modified it to confirm his findings, and it turned out it was indeed the case.
Our hero had damaged the dragon and it was time for a celebration.

He went out and acquired some mead which he ingested while he typed up his story and waited for the dragon to finally die (the commit caused a mass-rebuild, I had to wait for my repro).

In the end all was good in [NixOps-AWS](https://github.com/nixos/nixops-aws)-town and type checks could run. (PR for that incoming tomorrow).

(cherry picked from commit d88a773)
The prefix will now be correct in case of Nix env.

Note, however, that creating a venv from a Nix env still does not function. This does not seem to be possible
with the current approach either, because venv will copy or symlink our Python wrapper. In case it symlinks
(the default) it won't see a pyvenv.cfg. If it is copied I think it should function but it does not...

(cherry picked from commit 7447fff)
@adisbladis adisbladis changed the base branch from release-20.03 to staging-20.03 May 1, 2020 18:06
@adisbladis
Copy link
Member Author

best send this to staging-20.03

Ok! Done.

@ofborg ofborg bot requested review from lovek323, globin, edolstra and fpletz May 1, 2020 19:14
@veprbl veprbl added this to Ready in Staging (stable) May 2, 2020
@adisbladis adisbladis merged commit 9c3d0d4 into NixOS:staging-20.03 May 2, 2020
Staging (stable) automation moved this from Ready to Done May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants