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: wxPython: init at 4.0.6 #64047

Closed
wants to merge 1 commit into from
Closed

python: wxPython: init at 4.0.6 #64047

wants to merge 1 commit into from

Conversation

tbenst
Copy link
Contributor

@tbenst tbenst commented Jul 1, 2019

Motivation for this change

add wxPython 4, which is unlisted in python-modules and broken.

Things done

Split off from #61253 and addressed comments by @veprbl

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Resolves: #55426

@tbenst tbenst requested a review from FRidh as a code owner July 1, 2019 18:53
@tbenst
Copy link
Contributor Author

tbenst commented Jul 1, 2019

also of interest to @deliciouslytyped, sorry for delay!

Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

This should build for python2 according to pypi, please try to build that as well

pkgs/development/python-modules/wxPython/4.0.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/wxPython/4.0.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/wxPython/4.0.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/wxPython/4.0.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/wxPython/4.0.nix Outdated Show resolved Hide resolved
gtk3 libjpeg libtiff SDL gst-plugins-base libnotify freeglut xorg.libSM
which
];
nativeBuildInputs = [ pkgconfig which doxygen wxGTK libX11 ];
Copy link
Member

Choose a reason for hiding this comment

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

Like I said before, wxGTK and libX11 are libraries, so let's move them to buildInputs if they are needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@veprbl if I do this, the build fails shrug

Copy link
Member

Choose a reason for hiding this comment

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

The build succeeds if I remove libX11. Moving wxGTK, indeed, fails because it can't find "wx-config", which I don't understand ATM.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still matter? I'm going back to take another shot at enabling tests and I might look into this or add it as an open problem.

Copy link
Member

Choose a reason for hiding this comment

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

It is probably okay now

nativeBuildInputs = [ pkgconfig which doxygen wxGTK ];
buildInputs = [ libjpeg libtiff SDL
gst-plugins-base libnotify freeglut xorg.libSM ncurses
requests libpng gstreamer libX11
pathlib2
(wxGTK.gtk)
]
++ lib.optional openglSupport pyopengl;

@tbenst tbenst force-pushed the wxPython branch 2 times, most recently from 85dfa10 to deaf7e3 Compare July 2, 2019 07:18
@tbenst
Copy link
Contributor Author

tbenst commented Jul 2, 2019

@veprbl @jonringer thx for great comments! I took suggestions as possible, except where it would break the build

'';

buildPhase = ''
python build.py -v --use_syswx dox etg --nodoc sip build_py
Copy link
Member

Choose a reason for hiding this comment

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

Please build a wheel using bdist_wheel. The installPhase then won't need to be overridden.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @FRidh! When I add python build.py bdist_wheel to the end of buildPhase, I get:

File "setup.py", line 15, in <module>
    from setuptools                     import setup, find_packages
ModuleNotFoundError: No module named 'setuptools'

Per the manual, I tried adding setuptools_scm to nativeBuildInputs and setuptools to propogatedBuildInputs, but the error is unchanged.

Copy link
Contributor

Choose a reason for hiding this comment

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

The one time I had an error like that I think something was wrong with how PYTHONPATH was set up (determined by comparing the failing version to something else maybe?), but I don't think I spent the time to figure it out.

Copy link
Member

Choose a reason for hiding this comment

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

They are using a custom build system based on waf so I don't think our generic builders will be able to handle this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I worked on my own version of this PR a while ago, and was able to get bdist_wheel working using this: lopsided98@47786de#diff-5dbc383e8d8890c6c54121d966199558R31

@deliciouslytyped
Copy link
Contributor

ping?

nativeBuildInputs = [
pkgconfig
];
doCheck = false;
Copy link
Contributor

@deliciouslytyped deliciouslytyped Jul 26, 2019

Choose a reason for hiding this comment

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

Why? I haven't tried it yet, just asking.

'';

installPhase = ''
${python.interpreter} setup.py install --skip-build --prefix=$out
Copy link
Contributor

@deliciouslytyped deliciouslytyped Jul 26, 2019

Choose a reason for hiding this comment

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

This uses python.interpreter but buildPhase just calls python? Why the difference?
I don't know what this does offhand but maybe this has something to do with the setuptools error? (or maybe not)

Copy link
Member

Choose a reason for hiding this comment

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

In our generic builder we use a following:

${python.pythonForBuild.interpreter} nix_run_setup ${setupPyGlobalFlagsString} ${setupPyBuildExtString} bdist_wheel

wrapPythonPrograms
'';

passthru = { inherit wxGTK openglSupport; };
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Jul 28, 2019

@veprbl I'm trying to enable tests but working on it is really frustrating when a failure in the test phase fails the whole derivation and I have to wait an hour for the next cycle. Is there any way to separate tests and the build into separate derivations?

Edit: meanwhile I worked around this by using nix-shell.

Copy link
Contributor

@lopsided98 lopsided98 left a comment

Choose a reason for hiding this comment

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

I worked on my own version of this PR a while ago but hadn't gotten around to submitting it. Overall, this PR seems better (it doesn't rely on wx-config and builds the documentation), but there might a few parts of my version that you could use.

@@ -4660,6 +4660,11 @@ in {
inherit (pkgs) pkgconfig;
};

wxPython_4_0 = callPackage ../development/python-modules/wxPython/4.0.nix {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should either follow the naming convention of wxPython30, or wxPython30 should be renamed (with an alias for compatibility).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, since wxPython 3.0 does not support Python 3, we might want to make the wxPython package point to 4.0 on Python 3. Even if some packages aren't compatible, it seems better than having every package that uses wxPython fail to evaluate on Python 3.

Copy link
Contributor

@deliciouslytyped deliciouslytyped Jul 28, 2019

Choose a reason for hiding this comment

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

Is WxPython 4 API compatible with 3?
Edit: I suppose python 3 apps wont have been coded to use wxpython 3 anyway though because there was no support for wxpython in python 3 before... I think what I was trying to say is: is incompatibility even a thing?

Copy link
Contributor

Choose a reason for hiding this comment

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

They are not fully API compatible. Any application that wants to target both Python 2 and 3 is probably going to use wxPython 4.0, but it might be possible to be compatible with both 3.0 and 4.0.

Making 4.0 the default for Python 3 will only make packages that currently fail to evaluate, fail to build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lopsided98 see #64047 (comment)

I agree re pointing wxPython to 4.0 for python3..do you know how to do this? I’m familiar with overrides but not with how to do this in nixpkgs

'';

buildPhase = ''
python build.py -v --use_syswx dox etg --nodoc sip build_py
Copy link
Contributor

Choose a reason for hiding this comment

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

I worked on my own version of this PR a while ago, and was able to get bdist_wheel working using this: lopsided98@47786de#diff-5dbc383e8d8890c6c54121d966199558R31

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Jul 28, 2019

@lopsided I can't reply to your post for some reason but the original buildPhase has similar code to that

(which I assume you based off of) so...kind of a bad phase override (or this stuff didn't exist at the time?).

@jonringer
Copy link
Contributor

jonringer commented Jul 28, 2019

@veprbl I'm trying to enable tests but working on it is really frustrating when a failure in the test phase fails the whole derivation and I have to wait an hour for the next cycle.

nix-shell ./path/to/nixpkgs/default.nix -A pythonPackage.wxPython will probably your best bet, you will be loaded into a similar environment as when the package builds, I don't remember if checkInputs all get inserted. If you have the source checked out locally, then you can iterate within that shell.

Edit: meanwhile I worked around this by using nix-shell.

Was just making sure :)

Another option would be to make 2 derivations, one for the building the initial package, then another that imports that package and runs the tests

@lopsided98
Copy link
Contributor

@lopsided I can't reply to your post for some reason but the original buildPhase has similar code to that

Yes, I adapted mine from that.

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Jul 28, 2019

It annoys me that you had to duplicate the code like that but I don't know what to do about it. :(
Meanwhile I had some trouble figuring out how the heck to run the unit tests. Maybe we should try to document somewhere that if anyone is trying to find something related to the build system they should look in build.py... (I completely forgot about that since the last time I poked at this)

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Jul 28, 2019

It appears to me that build.py does some stuff for calling python itself, for running tests - this may be dropping/mangling PYTHONPATH or whatever the nix infrastructure does. This may need to be worked around to get tests to function.

It might be good to get some wxPython people to review our stuff at some point?

Edit:
Temporary workaround: patch to the following:
os.environ['PYTHONPATH'] = os.environ['PYTHONPATH']+ ":" + phoenixDir()
Will ask upstream to patch.

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Jul 28, 2019

@jonringer Thanks, I totally missed your comment. I was thinking about making two derivations like you said but I thought of how to do it with nix-shell faster than otherwise. Now that I actually know how to run the tests I might try the latter though.

Currently stuck on /nix/store/dwi186iqm6p3kfncfsl62fbfwfry1yci-python3-3.7.3/bin/python3.7: can't open file 'nix_run_setup': [Errno 2] No such file or directory in nix-build, but maybe more on that later.
...Which might be exactly what @lopsided98 worked around as discussed above...

@deliciouslytyped
Copy link
Contributor

However, I don't understand why that would be the case given that both buildPhase and installCheckPhase (in my WIP) are overridden, so nothing should be trying to call nix_run_setup...

@jonringer
Copy link
Contributor

@deliciouslytyped your installCheckPhases are being overriden most likely because you're using a buildPythonPackage which will override those attrs here or a related build helper.

/nix/store/dwi186iqm6p3kfncfsl62fbfwfry1yci-python3-3.7.3/bin/python3.7: can't open file 'nix_run_setup'

This will happen if you override the buildPhase, but use the default checkPhase. If you're using installCheckPhase, i would move it to checkPhase. Or even better yet, try to use the preCheck, and postCheck hooks if it fits your problem domain.

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Jul 29, 2019

Doh, the order on the // there caught me with my pants down! (I assumed...) Looks like I need some more belts...

Seems a bit unorthodox that they go and override that...
Hmm, interesting, maybe that's a way to do it without using removeAttrs.
I'm too tired at this point to figure out if what I'm saying is coherent, so that's that for today.

@jonringer
Copy link
Contributor

@deliciouslytyped for the average python package, this behavior makes sense. The typical "build and install"for python is just dumping .py's into a directory. If you do need all the phases and "more" control, you could do stdenv.mkderivation + toPythonModule however, this is generally more aimed at projects that have a significant amount of native build outputs and the project stands on it's own outside of python; the toPythonModule will just package the bindings, IIRC.

@FRidh FRidh self-assigned this Jul 29, 2019
@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Jul 29, 2019

Edit: see also: https://github.com/wxWidgets/Phoenix/pull/1135/files for test thingy usage example

WIP, has tests at least running:

{ openglSupport ? true

, lib
, stdenv

, buildPythonPackage
, fetchPypi
, python
, pyopengl

, wxGTK
, cairo
, pango

#nativeBuildInputs
, which
, doxygen
, pkgconfig

#buildInputs
, libjpeg
, libtiff
, SDL
, gst-plugins-base
, libnotify
, freeglut
, xorg
, ncurses
, requests
, libpng
, gstreamer
, libX11
, pathlib2

#checkInputs #TODO clean
, numpy
, pytest, pytest_xdist, pytest-timeout
, sphinx
, xvfb_run
}@args:

let
  pname = "wxPython";
  version = "4.0.6";
in

buildPythonPackage rec {
  inherit pname version;

  src = fetchPypi {
    inherit pname version;
    sha256 = "35cc8ae9dd5246e2c9861bb796026bbcb9fb083e4d49650f776622171ecdab37";
    };

  checkInputs = [ xvfb_run sphinx numpy pytest pytest_xdist pytest-timeout ];
  disabledTests = [
    "access_Tests"  #TODO because not implemented yet and automatically fails
    "test_lib_agw_piectrlMethods"  #TODO because hangs the build and timeout skips the rest of tests
    ];
  checkPhase = ''
    runHook preCheck
    xvfb-run -s '-screen 0 800x600x24' \
      ${python.pythonForBuild.interpreter} build.py test --verbose --pytest_timeout=60 \
         --extra_pytest="-k 'not ( ${lib.concatStringsSep " or " disabledTests} )'"
    runHook postCheck
    '';

  nativeBuildInputs = [ pkgconfig which doxygen wxGTK ];

  buildInputs = [
    libjpeg libtiff SDL
    gst-plugins-base libnotify freeglut xorg.libSM ncurses
    requests libpng gstreamer libX11
    pathlib2
    (wxGTK.gtk)
    ] ++ (lib.optional openglSupport pyopengl);

  hardeningDisable = [ "format" ];
  
  DOXYGEN = "${doxygen}/bin/doxygen";

  preConfigure = lib.optionalString (!stdenv.isDarwin) ''
    substituteInPlace wx/lib/wxcairo/wx_pycairo.py \
      --replace 'cairoLib = None' 'cairoLib = ctypes.CDLL("${cairo}/lib/libcairo.so")'
    substituteInPlace wx/lib/wxcairo/wx_pycairo.py \
      --replace '_dlls = dict()' '_dlls = {k: ctypes.CDLL(v) for k, v in [
        ("gdk",        "${wxGTK.gtk}/lib/libgtk-x11-2.0.so"),
        ("pangocairo", "${pango.out}/lib/libpangocairo-1.0.so"), #TODO does this need to refer to .out
        ("appsvc",     None)
        ]}'
    '';

  prePatch = ''
    substituteInPlace build.py \
      --replace "os.environ['PYTHONPATH'] = phoenixDir()" "os.environ['PYTHONPATH'] = os.environ['PYTHONPATH'] + ':' + phoenixDir()"
    '';

  buildPhase = ''
    python build.py -v --use_syswx dox etg --nodoc sip build_py
    '';

  installPhase = ''
    ${python.interpreter} setup.py install --skip-build --prefix=$out #TODO this uses python.interpreter but the above does not?
    wrapPythonPrograms
    '';

  passthru = { inherit args; };

  meta = {
    description = "Cross platform GUI toolkit for Python, Phoenix version";
    homepage = http://wxpython.org/;
    license = lib.licenses.wxWindows;
    };

  }

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Jul 29, 2019

Something looks sketchy here:

unittests/test_lib_agw_piectrl.py::lib_agw_piectrl_Tests::test_lib_agw_piectrlCtor PASSED [ 44%]
INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File "/nix/store/9pl2hgg3p2ijhzaycj2h77p3ny0vwfgy-python3.7-pytest-4.2.1/lib/python3.7/site-packages/_pytest/main.py", line 203, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>   File "/nix/store/9pl2hgg3p2ijhzaycj2h77p3ny0vwfgy-python3.7-pytest-4.2.1/lib/python3.7/site-packages/_pytest/main.py", line 243, in _main
INTERNALERROR>     config.hook.pytest_runtestloop(session=session)

[SNIP]

INTERNALERROR>     pytest.fail('Timeout >%ss' % timeout)
INTERNALERROR>   File "/nix/store/9pl2hgg3p2ijhzaycj2h77p3ny0vwfgy-python3.7-pytest-4.2.1/lib/python3.7/site-packages/_pytest/outcomes.py", line 113, in fail
INTERNALERROR>     raise Failed(msg=msg, pytrace=pytrace)
INTERNALERROR> Failed: Timeout >60.0s

== 19 failed, 754 passed, 6 skipped, 1 xfailed, 68 warnings in 524.68 seconds ==

Does it just abort the rest of the test process on a timeout? Note the last PASSED is at 44%.

Edit: The help string does say "Timeout, in seconds, for stopping stuck test cases. (Currently not working as expected, so disabled by default.)"

@tbenst
Copy link
Contributor Author

tbenst commented Jul 31, 2019

@deliciouslytyped thanks for picking up, been offline in the wilderness. Will try to look at tests in a couple days, they are a real PITA which is why I disabled doCheck. I did try some applications though, like Helloworld, that worked fine.

@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/wxpython-on-python3/3588/2

@deliciouslytyped
Copy link
Contributor

Test termination behavior of timeout is explained at https://pypi.org/project/pytest-timeout/

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Jul 31, 2019

@FRidh stuff seems to generally work, how should failing tests be handled? It would be nice to get this merged finally...

I'm also not sure how to process test ouput.

@@ -4660,6 +4660,11 @@ in {
inherit (pkgs) pkgconfig;
};

wxPython_4_0 = callPackage ../development/python-modules/wxPython/4.0.nix {
inherit (pkgs) pkgconfig;
Copy link
Contributor

@deliciouslytyped deliciouslytyped Aug 2, 2019

Choose a reason for hiding this comment

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

Why? (Well ok this isn't specific to this PR)

Copy link
Member

Choose a reason for hiding this comment

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

The reason why we need to explicitly pass things from pkgs is because the callPackage defined in python-packages.nix

callPackage = pkgs.newScope self;

will only substitute arguments from self (aka pythonPackages) and not from pkgs.

Copy link
Contributor

Choose a reason for hiding this comment

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

pythonPackages seems to have its own pkgconfig attribute too somehow? Meanwhile pkgs has pkg-config... (or I got mixed up somewhere)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok so pkgs has a pkgconfig and a pkg-config, and pyhtonPackages has a pkgconfig.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so my explanation was wrong. The callPackage will also substitute arguments from pkgs, but self (pythonPackages) takes priority. And, indeed we already have pkgconfig attribute in pythonPackages, but we don't want to get the "pkgconfig" python package, we want just the regular pkgconfig. And this is why we need to explicitly state that it must come from pkgs.

Copy link
Contributor

@jonringer jonringer Aug 2, 2019

Choose a reason for hiding this comment

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

Correct, you should be able to import something like "tree", even though it doesnt exist in top-level/python-modules.nix, because it inherits from pkgs. However, if you do something like "docker" you'll get pythonPackages.docker instead of ./nixpkgs -A docker due to how callPackage prioritizes overlapping attrs.

@jonringer
Copy link
Contributor

@deliciouslytyped you can use the pytest testrunner, it has more options to disabling certain tests or files.

pytest --ignore=path/to/file.py or -k 'not some_test_substring'

example can be seen here 93ebc61

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Aug 10, 2019

@FRidh can you please set some minumum criteria for getting this merged so I have something to work towards?
We can set some additional goals for after that, I would really like this to just be in the tree already.

@deliciouslytyped
Copy link
Contributor

Can/should we do anything about using wrapgappshook here? or does that need to be used in the libraries/binaries that consume this package?

@FRidh FRidh added this to the 19.09 milestone Aug 18, 2019
@FRidh
Copy link
Member

FRidh commented Aug 18, 2019

I made a change and pushed to master. Thanks for the effort. In the future, though, we should improve this to build a wheel instead.

@FRidh FRidh closed this Aug 18, 2019
@deliciouslytyped
Copy link
Contributor

Yay, since this PR is closed, I should probably create a TODO list in a new issue "soon".

@deliciouslytyped
Copy link
Contributor

What did you change?

@FRidh
Copy link
Member

FRidh commented Aug 18, 2019

Using ${python.interpreter} instead of python

@tbenst tbenst deleted the wxPython branch August 18, 2019 22:37
@deliciouslytyped
Copy link
Contributor

I'm not volunteering right now but does anyone want to work on getting tests enabled and following up with upstream about the tests?

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.

Link failure in wxWidgets component when building wxPython 4.0
7 participants