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

python3Packages.wxpython_4_1: init at 4.1.0 #95462

Closed
wants to merge 1 commit into from

Conversation

@tfmoraes
Copy link
Contributor

@tfmoraes tfmoraes commented Aug 14, 2020

Motivation for this change

Add WXPython-4.1.0 to NixOS. The package is compiling its own wxgtk because wxpython is not compiling with it.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
$ nix build --no-link --keep-going --option build-use-sandbox relaxed -f /home/thiago/.cache/nixpkgs-review/rev-7133bbdbd31696c8a646ea44e85e78f38be3ca2d/build.nix
[2 built, 0.0 MiB DL]
2 packages built:
python37Packages.wxPython_4_1 python38Packages.wxPython_4_1

$ nix-shell /home/thiago/.cache/nixpkgs-review/rev-7133bbdbd31696c8a646ea44e85e78f38be3ca2d/shell.nix

  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Copy link
Contributor

@jonringer jonringer left a comment

looks like we already have a wxPYTHON_4_0, it would probably be best to only keep one of each major version (unless the package causes breakages between minor versions as well)

pkgs/development/python-modules/wxPython/4.1.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/wxPython/4.1.nix Outdated Show resolved Hide resolved
@tfmoraes
Copy link
Contributor Author

@tfmoraes tfmoraes commented Aug 15, 2020

looks like we already have a wxPYTHON_4_0, it would probably be best to only keep one of each major version (unless the package causes breakages between minor versions as well)

There are a lot of change between wxpython4.0.x and wxpython4.1.x. For instance wxWidgets/Phoenix#1612

@tfmoraes
Copy link
Contributor Author

@tfmoraes tfmoraes commented Aug 15, 2020

❯ nix-shell -p nixpkgs-review --run "nixpkgs-review rev cc8fd8e62606b751555615985b9af5dc58a79dec"                                                                    
$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0
$ git worktree add /home/thiago/.cache/nixpkgs-review/rev-cc8fd8e62606b751555615985b9af5dc58a79dec/nixpkgs 99d379c45c793c078af4bb5d6c85459f72b1f30b
Preparing worktree (detached HEAD 99d379c45c7)
HEAD is now at 99d379c45c7 Merge pull request #79874 from oxzi/tinycbor-v0.5.3
$ nix-env -f /home/thiago/.cache/nixpkgs-review/rev-cc8fd8e62606b751555615985b9af5dc58a79dec/nixpkgs -qaP --xml --out-path --show-trace
$ git merge --no-commit cc8fd8e62606b751555615985b9af5dc58a79dec
Auto-merging pkgs/top-level/python-packages.nix
Automatic merge went well; stopped before committing as requested
$ nix-env -f /home/thiago/.cache/nixpkgs-review/rev-cc8fd8e62606b751555615985b9af5dc58a79dec/nixpkgs -qaP --xml --out-path --show-trace --meta
2 packages added:
python37Packages.wxPython_4_1 (init at 4.1.0) python38Packages.wxPython_4_1 (init at 4.1.0)

$ nix build --no-link --keep-going --option build-use-sandbox relaxed -f /home/thiago/.cache/nixpkgs-review/rev-cc8fd8e62606b751555615985b9af5dc58a79dec/build.nix
[3 built, 0.0 MiB DL]
2 packages built:
python37Packages.wxPython_4_1 python38Packages.wxPython_4_1

@evils
Copy link
Member

@evils evils commented Sep 24, 2020

does this package expose the wxWidgets version it built?
in kicad for example, it'd be nice to be able to use the version of wxWidgets built by this, because it seems to require those packages' versions to match.
(https://gitlab.com/kicad/code/kicad/-/issues/4431)

@tfmoraes
Copy link
Contributor Author

@tfmoraes tfmoraes commented Sep 24, 2020

@evils it doesn't. I have this pull request to package wx3.1.4 #95460 . In this commit wxPython starts to use wx3.1.4 wxWidgets/Phoenix@57702fa#diff-abf77184f55403d75b9d51d79162a7ca

@evils
Copy link
Member

@evils evils commented Sep 24, 2020

In this commit wxPython starts to use wx3.1.4 wxWidgets/Phoenix@57702fa#diff-abf77184f55403d75b9d51d79162a7ca

i'm not sure why you're mentioning that
as this is after wxPython 4.1.0
and they've since advanced the dependency to wxWidgets master again

@tfmoraes
Copy link
Contributor Author

@tfmoraes tfmoraes commented Sep 24, 2020

@evils I was thinking in package that git-committed version of wxpython, but it's a bad idea. But I managed to compile wxPython-4.1.0 with wxWidgets-3.1.4 in my branch here https://github.com/tfmoraes/nixpkgs/tree/wxgtk314_wxpython411 . Maybe if fix your problem.

@evils
Copy link
Member

@evils evils commented Sep 25, 2020

@tfmoraes your wxPython 4.1.0 with wxWidgets 3.1.4 branch solves kicad issue 4431 :D

@tfmoraes
Copy link
Contributor Author

@tfmoraes tfmoraes commented Sep 25, 2020

@evils @jonringer what is better?

  1. Update wxPython and wxWidgets PR and keep 2 PR?
  2. Create a new PR with wxPython and wxWidgets together

@zowoq zowoq removed their request for review Sep 26, 2020
@jonringer
Copy link
Contributor

@jonringer jonringer commented Sep 27, 2020

I would just combine the two PRs, you can use either, just make a comment saying what you decide

@evils
Copy link
Member

@evils evils commented Sep 27, 2020

Since this seems to copy the previous wxPython package, have you taken into account #94108?
A lot of the previous package's dependencies were unneeded

("appsvc", None)
]}'
# https://github.com/wxWidgets/Phoenix/pull/1584
Copy link
Member

@evils evils Sep 27, 2020

it's probably better to refer to wxWidgets/Phoenix#1699 as that indicates this change has been upstreamed and can probably be removed at a later point

Copy link
Contributor Author

@tfmoraes tfmoraes Sep 27, 2020

Done, in the other PR #98951

@tfmoraes
Copy link
Contributor Author

@tfmoraes tfmoraes commented Sep 27, 2020

I would just combine the two PRs, you can use either, just make a comment saying what you decide

Hi @jonringer, I created a new PR with wxWidgets + wxPython #98951

@tfmoraes
Copy link
Contributor Author

@tfmoraes tfmoraes commented Sep 27, 2020

Since this seems to copy the previous wxPython package, have you taken into account #94108?
A lot of the previous package's dependencies were unneeded

Yes, @evils in the other PR with both changes #98951

@tfmoraes tfmoraes closed this Oct 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment