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

Fix linking against Homebrew's Tcl/Tk 8.6.13 in MacOS #2820

Merged
merged 9 commits into from Oct 15, 2023

Conversation

startergo
Copy link
Contributor

@startergo startergo commented Oct 15, 2023

Correct the path for tcl-tk include for homebrew

Make sure you have checked all steps below.

Prerequisite

  • Please consider implementing the feature as a hook script or plugin as a first step.
    • pyenv has some powerful support for plugins and hook scripts. Please refer to Authoring plugins for details and try to implement it as a plugin if possible.
  • Please consider contributing the patch upstream to rbenv, since we have borrowed most of the code from that project.
    • We occasionally import the changes from rbenv. In general, you can expect changes made in rbenv will be imported to pyenv too, eventually.
    • Generally speaking, we prefer not to make changes in the core in order to keep compatibility with rbenv.
  • My PR addresses the following pyenv issue (if any)

Description

Tests

  • My PR adds the following unit tests (if any)
pyenv install 2.7.18
python -m Tkinter

Correct the path for `tcl-tk` `include` for homebrew
plugins/python-build/bin/python-build Outdated Show resolved Hide resolved
…thon-build/bin/find / -name tcl.h 2>/dev/null

Add conditional coding of `include` folder rather than hardcoded one.
Copy link
Contributor Author

@startergo startergo left a comment

Choose a reason for hiding this comment

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

Added conditional coding of include folder rather than hardcoded one

Copy link
Contributor Author

@startergo startergo left a comment

Choose a reason for hiding this comment

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

tcl.h can be in multiple locations. How can we ensure tcl-tk from Homebrew is used in the use_homebrew_tcltk()?

@native-api
Copy link
Member

native-api commented Oct 15, 2023

This still links 2.7.18 against 8.5 . As I said, 2.7.18's Tkinter build is bugged, it appends rather than prepends the required directory to the compiler search path -- so it doesn't pick the intended version when other directories or the compiler search path also have it.

Looks like to work around that, we'd need to include the mode to configure Tcl/Tk via CPPFLAGS/LDFLAGS rather than through --with-tcltk-*.


However.

I've looked through the history... and we seem to have never done this before! So, configuring 2.7 with Homebrew's Tcl/Tk never worked (at least since the time Xode SDK had it bundled)! I was suspecting that this worked before and then we broke it -- then this would be a bugfix.
But it's not. This'll be a new feature.

And because 2.7 is EOL, adding new features just for it would be against our Deprecation policy.


So I suggest that we should rather make a patch for 2.7.18 that would properly prepend the dirs to the compiler's search paths.

@native-api native-api changed the title Update python-build Fix linking against Homebrew's Tcl/Tk for 2.7.18 Oct 15, 2023
@native-api native-api changed the title Fix linking against Homebrew's Tcl/Tk for 2.7.18 Fix linking against Homebrew's Tcl/Tk for 2.7.18 in MacOS Oct 15, 2023
@native-api
Copy link
Member

native-api commented Oct 15, 2023

And because 2.7 is EOL, adding new features just for it would be against our Deprecation policy.

That said, PyPy-2.7 is NOT EOL -- so if this feature applies to it as well, it's fine to add.

@startergo
Copy link
Contributor Author

This still links 2.7.18 against 8.5

True, but at least it is not borked. Interestingly if I hardcode it like this:

package_option python configure --with-tcltk-includes="-I$tcltk_libdir/include/tcl-tk"

it links 2.7.18 against 8.6, but if I do conditional setup like this:

if [ -d "$tcltk_libdir/include/tcl-tk" ]; then
   package_option python configure --with-tcltk-includes="-I$tcltk_libdir/include/tcl-tk"
fi
package_option python configure --with-tcltk-includes="-I$tcltk_libdir/include"	

It links against 8.5. Go figure...

@startergo
Copy link
Contributor Author

Looks like to work around that, we'd need to include the mode to configure Tcl/Tk via CPPFLAGS/LDFLAGS rather than through --with-tcltk-*.

Isn't it better to rely on pkg-config and include it in the brew requirement? At least it would use relative paths.

Copy link
Contributor Author

@startergo startergo left a comment

Choose a reason for hiding this comment

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

aah! my bad...

@native-api
Copy link
Member

Wow! I don't know how but it works.

% find ~/.pyenv/versions/2.7.18 -name _tkinter\* | xargs otool -L
/Users/admin/.pyenv/versions/2.7.18/lib/python2.7/lib-dynload/_tkinter.so:
	/usr/local/opt/tcl-tk/lib/libtcl8.6.dylib (compatibility version 8.6.0, current version 8.6.13)
	/usr/local/opt/tcl-tk/lib/libtk8.6.dylib (compatibility version 8.6.0, current version 8.6.13)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1319.0.0)

@native-api native-api changed the title Fix linking against Homebrew's Tcl/Tk for 2.7.18 in MacOS Fix linking against Homebrew's Tcl/Tk 8.6.13 in MacOS Oct 15, 2023
@native-api native-api merged commit d25cd0d into pyenv:master Oct 15, 2023
Copy link
Contributor Author

@startergo startergo left a comment

Choose a reason for hiding this comment

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

Makes sense. Thanks.

@native-api
Copy link
Member

Thanks to you, too! Way to prove me wrong!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RuntimeError: tk.h version (8.5) doesn't match libtk.a version (8.6) python 2.7.18
2 participants