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: use separate output for tkinter #19309

Merged
merged 8 commits into from
Oct 13, 2016
Merged

Python: use separate output for tkinter #19309

merged 8 commits into from
Oct 13, 2016

Conversation

FRidh
Copy link
Member

@FRidh FRidh commented Oct 6, 2016 •
edited
Loading

Motivation for this change

See issue #19255 and #1819.

  • Unify expressions of different interpreters
  • Reduce closure size of Python 3.x interpreters
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
    • OS X
    • 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 FRidh added 6.topic: python 1.severity: mass-rebuild This PR causes a large number of packages to rebuild labels Oct 6, 2016
@mention-bot
Copy link

@FRidh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @chaoflow and @domenkozar to be potential reviewers.

@FRidh FRidh changed the title Outputs Python: use separate output for tkinter Oct 6, 2016
@FRidh FRidh added the 2.status: work-in-progress This PR isn't done label Oct 7, 2016
@FRidh FRidh force-pushed the outputs branch 4 times, most recently from d728388 to 5d41890 Compare October 10, 2016 07:49
@FRidh FRidh removed the 2.status: work-in-progress This PR isn't done label Oct 10, 2016
@FRidh FRidh added the 2.status: work-in-progress This PR isn't done label Oct 10, 2016
@FRidh
Copy link
Member Author

FRidh commented Oct 10, 2016

Sigh... putting curses in a separate output won't help because python and libpython directly link against it.

@FRidh FRidh force-pushed the outputs branch 2 times, most recently from 4d3fc5a to 1c72375 Compare October 10, 2016 08:10
@FRidh FRidh removed the 2.status: work-in-progress This PR isn't done label Oct 10, 2016
@FRidh FRidh changed the base branch from master to staging October 10, 2016 08:28
@FRidh FRidh changed the base branch from staging to master October 10, 2016 08:34
This was referenced Oct 10, 2016
Closed
Closed
@domenkozar
Copy link
Member

@FRidh that's fine I think. It's not ideal, but this way Python will be way less painful on Nix.

@vcunat
Copy link
Member

vcunat commented Oct 13, 2016

So all those remaining sqlite3 mentions should also get removed? This blocks staging evaluation ATM.

@FRidh
Copy link
Member Author

FRidh commented Oct 13, 2016

@vcunat yes they should. I thought I got them all.

@FRidh
Copy link
Member Author

FRidh commented Oct 13, 2016

ahh yes, it wasn't just pythonPackages.modules.sqlite3 but also pythonPackages.sqlite3. I forgot those.

@FRidh
Copy link
Member Author

FRidh commented Oct 13, 2016

I'm sure I've asked this before but how can one test whether the whole of Nixpkgs evaluates?

@vcunat
Copy link
Member

vcunat commented Oct 13, 2016

There are also python27Full and curses, maybe more.

@vcunat
Copy link
Member

vcunat commented Oct 13, 2016

@FRidh: for quick eval I regularly use

nix-env -f . -qaP --meta --json --drv-path --show-trace >/dev/null

But one can do nix-build pkgs/top-level/release.nix -A tarball -Q to be proper (all Hydra platforms etc.).

@FRidh
Copy link
Member Author

FRidh commented Oct 13, 2016 •
edited
Loading

Thanks @vcunat . That should be it (efb6052)

@vcunat
Copy link
Member

vcunat commented Oct 13, 2016

No, there were quite a few more: 6eeea6e.

@FRidh
Copy link
Member Author

FRidh commented Oct 14, 2016

hmm, it seemed to evaluate. But thank you for fixing the (hopefully) last ones.

@domenkozar domenkozar mentioned this pull request Oct 14, 2016
Open
37 tasks
@FRidh FRidh mentioned this pull request Oct 14, 2016
Closed
@domenkozar
Copy link
Member

domenkozar commented Oct 14, 2016 •
edited
Loading

Thank you @FRidh for doing this ❤️

This was referenced Oct 14, 2016
Closed
Closed
@copumpkin
Copy link
Member

copumpkin commented Oct 16, 2016 •
edited
Loading

Argh, a complication: the darwin stdenv bootstrap depends on python (because LLVM's build process uses it) and this change massively slowed down our bootstrap process, since it now needs to build a ton of TK stuff, multiple times since we have multiple pythons to kill dependencies on the bootstrap tools 😦

Can we make the TK stuff optional so I can ask for a TK-free python during bootstrap? Then only the final "userspace" one would get TK.

@copumpkin
Copy link
Member

Also, it actually seems to have broken the python build, which prevents bootstrap 😢

@vcunat
Copy link
Member

vcunat commented Oct 16, 2016 •
edited
Loading

@copumpkin: I think that should be solved by using pythonSmall for anything needed during bootstrapping, as that one is without X, tcl and tk. EDIT: we should probably switch all those packages to it regardless of platform.

@FRidh
Copy link
Member Author

FRidh commented Oct 16, 2016

I am actually thinking of getting rid of the tkinter output again, and just
build without tkinter by default, and then build once with tkinter and copy
the tkinter module in another derivation. Then we can get rid of
pythonSmall.

On Oct 16, 2016 10:00 AM, "Vladimír Čunát" [email protected] wrote:

@copumpkin https://github.com/copumpkin: I think that should be solved
by using pythonSmall for bootstrapping, as that one is without X, tcl and
tk.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#19309 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACB872EUsaiKFngXWl_w_-ylPl14we98ks5q0dmJgaJpZM4KQTvy
.

@FRidh FRidh mentioned this pull request Oct 16, 2016
Merged
7 tasks
@FRidh
Copy link
Member Author

FRidh commented Oct 16, 2016

@vcunat @copumpkin see #19594 , I think it's a better solution than what I merged earlier.

FRidh added a commit to FRidh/nixpkgs that referenced this pull request Oct 16, 2016
@FRidh
Python: remove pythonSmall
107c035 
In NixOS#19309 a separate output for tkinter was added.

Several dependencies of Python depend indirectly on Python. We have the
following two paths:
```
‘python-2.7.12’ - ‘tk-8.6.6’ - ‘libXft-2.3.2’ - ‘libXrender-0.9.10’ -
‘libX11-1.6.4’ - ‘libxcb-1.12’ - ‘libxslt-1.1.29’- ‘libxml2-2.9.4’ -
‘python-2.7.12’

‘python-2.7.12’ - ‘tk-8.6.6’ - ‘libXft-2.3.2’ - ‘fontconfig-2.12.1’ -
‘dejavu-fonts-2.37’ - ‘fontforge-20160404’ - ‘python-2.7.12’
```
Because only `tkinter` needs this, I added
```
pythonSmall = python.override {x11Support = false;};
```
to break the infinite recursion. We also still have the output
`tkinter`.

However, we might as well build without x11Support by default. Then we build with x11Support as well so we get the tkinter module and put that in a separate package.
@copumpkin
Copy link
Member

Dammit, I was just reaching for pythonSmall during bootstrap 😄 I don't really have a strong opinion on how Python actually works but I'll take a look at the other PR and opine if I have something useful to say.

@FRidh FRidh deleted the outputs branch July 31, 2017 19:02
@flokli flokli mentioned this pull request Aug 17, 2019
Merged
10 tasks
This was referenced Jan 9, 2021
Closed
Closed
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/python3x-vs-python3xfull-package-name-change-in-2016-vs-standardized-semantic-adopt-to-standard-python-semantic/10896/1

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/help-setting-up-nix-shell-with-python-and-vulkan/14512/2

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

@copumpkin copumpkin copumpkin left review comments

Assignees
No one assigned
Labels
1.severity: mass-rebuild This PR causes a large number of packages to rebuild 6.topic: python
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@FRidh @mention-bot @domenkozar @RonnyPfannschmidt @FlorentBecker @copumpkin @vcunat @nixos-discourse @bjornfor