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

Revert "python3: don't patch out -Wl,-stack_size,1000000" #166516

Closed
wants to merge 1 commit into from

Conversation

malob
Copy link
Member

@malob malob commented Mar 31, 2022

Description of changes

This reverts commit c7c3187, which had the following comment:

The original motivation behind removing that was to appease a consumer
of python-config's output. That issue was probably resolved by now, so
let's bring the build in sync with what python is doing by default.

There is at least one package (kitty) that is failing to build as a result of this change (see #165387).

I don't know much about this area, so reverting this commit is my naive way or resolving the issue.

Closes #165387.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give @veprbl some days to reply. If there is no reply we can merge.

@veprbl
Copy link
Member

veprbl commented Mar 31, 2022

I don't think that this is a good way. The straightforward revert will break python310 again and move us away from what the vanilla python does. It would seem like the kitty just should not use this flag to link the shared libraries. Someone could check if Apple's python-config reports the -stack_size flag, and if some other python on macOS does that.

@malob
Copy link
Member Author

malob commented Mar 31, 2022

FWIW, python310 does build for me on aarch64-darwin with the commit reverted:

~/C/nixpkgs on revert-c7c3187 nix run .#python310 -- --version
Python 3.10.3

I'll check on the config for python for various distributions on macOS now.

@malob
Copy link
Member Author

malob commented Mar 31, 2022

Version packaged with macOS:

/usr/bin/python3 -c "import sysconfig; print(sysconfig.get_config_var('LINKFORSHARED'))"
-Wl,-stack_size,1000000  -framework CoreFoundation Python3.framework/Versions/3.8/Python3

Version installed by Homebrew:

/opt/homebrew/bin/python3 -c "import sysconfig; print(sysconfig.get_config_var('LINKFORSHARED'))"
-Wl,-stack_size,1000000  -framework CoreFoundation /opt/homebrew/opt/python@3.9/Frameworks/Python.framework/Versions/3.9/Python

I was surprised by the latter since, I remember the maintainer of kitty saying folks should use Homebrew's Python to build kitty to get around the -stack_size issue.

I'll investigate a little more.

@veprbl
Copy link
Member

veprbl commented Mar 31, 2022

FWIW, python310 does build for me on aarch64-darwin with the commit reverted:

~/C/nixpkgs on revert-c7c3187 
❯ nix run .#python310 -- --version
Python 3.10.3

I'll check on the config for python for various distributions on macOS now.

Okay, I misremembered, you need to also revert the 49a0059 to trigger the bug.

@malob
Copy link
Member Author

malob commented Mar 31, 2022

Alrighty, after doing some more investigation, I now think that reverting this commit is the wrong approach, and that a patch to kitty in nixpkgs is the right call until things are addressed upstream. I've tried to start that conversation on the upstream repo here: kovidgoyal/kitty#289 (comment)

@malob malob closed this Mar 31, 2022
@malob malob deleted the revert-c7c3187 branch April 4, 2023 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants