-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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 tix module #21643
Python tix module #21643
Conversation
@symphorien, thanks for your PR! By analyzing the history of the files in this pull request, we identified @FRidh, @domenkozar and @kragniz to be potential reviewers. |
@@ -121,7 +122,7 @@ let | |||
|
|||
buildInputs = | |||
optional (stdenv ? cc && stdenv.cc.libc != null) stdenv.cc.libc ++ | |||
[ bzip2 openssl zlib ] | |||
[ bzip2 openssl zlib makeWrapper] | |||
++ optionals stdenv.isCygwin [ expat libffi ] | |||
++ [ db gdbm ncurses sqlite readline ] | |||
++ optionals x11Support [ tcl tk xlibsWrapper libX11 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ optionals x11Support [ tcl tk libX11 xproto tix ]
Then you should be able to get rid of the wrapper. At least, I made the change and it seems to work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure to understand well : Do you want me to put makeWrapper in [ tcl tk libX11 xproto tix ]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, put tix there like I wrote.
Ah, I made the change in python 3.5 expression, so that's why it looks slightly different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this http://xelpaste.net/2J0wKk but it doesn't work for me :
self.tk.eval('package require Tix')
_tkinter.TclError: can't find package Tix
license = with licenses; [ | ||
bsd2 # tix | ||
gpl2 # patches from portage | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you like to maintain this package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.
tix = tix-8_6; | ||
|
||
tix-8_6 = callPackage ../development/libraries/tix { }; | ||
tix-8_5 = callPackage ../development/libraries/tix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only use tix-8_6, so we don't need tix-8_5 and can just use
tix = callPackage ...
I don't think this will be a mass rebuild (tk isn't used much) but even so please rebase against staging. |
those are necessary to build some extensions like tix
Indeed, you are right. I checked the source code and it always tries to find the library using an environment variable. We shouldn't use a wrapper if we can do so without. I think we can patch the code to point to the Tix library. The following (Python 3.5) doesn't seem to be sufficient yet
because
Note that when using this method we should copy The Python docs write
|
The patch you proposed should have worked, but there was a mistake in the way I packaged tix for it to work. I force pushed something which is close to what you proposed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. I see you want to keep support for TIX_LIBRARY
? That seems reasonable.
@@ -113,6 +113,7 @@ let | |||
# will think the wrapper is the original. As long as the wrapper has | |||
# the same path as the original, this is OK. | |||
wrapProgram "$out/pypy-c/pypy-c" \ | |||
--set TIX_LIBRARY ${tix}/lib \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grepping the PyPy source code gave me the following file that needs to be patched
lib-python/2.7/lib-tk/Tix.py: tixlib = os.environ.get('TIX_LIBRARY')
There is one more challenge though. The default Python
because it uses the interpreter from |
I forgot pypy, that should be fixed. |
The parameter will stay; just the attibute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Note that I just saw it also links to the Python interpreter from pythonFull which is a bit of a problem.)
This has been fixed now in e276f59
@symphorien This PR looks good to me as it is. Unfortunately tix
won't work yet with python.withPackages(ps: [ps.tkinter])
. Even so I think this can go in now.
Motivation for this change
Being able to use the tkinter.tix python module (which is in the standard library).
This is done in three parts:
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)Regarding testing : I rebuilt my whole system with those versions of cpython (my laptop has not enough ram to build pypy) and it has been working fine since then. I could not test extensively all the packages impacted by those changes. And of course I checked that the tkinter.tix module works.