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

kicad: use the same wxGTK version for KiCad and for wxPython #98634

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@hannesweisbach
Copy link
Contributor

@hannesweisbach hannesweisbach commented Sep 24, 2020

Motivation for this change

Since wxPython is used in KiCad to enable scripting via Python I think the
right thing to do is to use the same version of wxGTK for both KiCad and
wxPython.

This gets interesting on macOS, where KiCad requires a specially patched version of wxwidgets (#98538)
See also #98450 and possibly #86040.

In the end this is to get KiCad to work on macOS (but I think things work on other OS' by accident).

My local build is still in progess … but maybe someone can tell me if and why I might be wrong in the mean time.

Maybe the right to do would be wxGTK = wxPython.wxGTK, as wxPython passes wxGTK through.

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"
  • 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.

python = python3;
wxPython = python.pkgs.wxPython_4_0;
wxPython = python.pkgs.wxPython_4_0.override { wxGTK = wxGTK; };
Copy link
Member

@evils evils Sep 24, 2020

this has the inconvenience that wxPython (at least currently) requires a specific revision of wxWidgets (known here as wxGTK)
optimally, the latest version of wxGTK would be the latest wxWidgets release (which it isn't, #95460)
and wxPython would use that (which it doesn't, see #95462 (that removes wxGTK from its inputs because it builds its own that isn't exposed for use in kicad?))

i'm hoping this mess clears up a bit on a subsequent wxPython release, though i still expect it to be tied to a specific release (hopefully) version of wxWidgets.

Copy link
Contributor Author

@hannesweisbach hannesweisbach Sep 24, 2020

Hm? wxPython just uses wxGTK30, same as KiCad in the stable = true configuration.
Or do you mean that stable = false implies also scripting = false, so that KiCad would not depend on wxPython when KiCad is built with a newer wxGTK, i.e. 3.1.x, that does not build/work with wxPython 4.0?
#95460 is about wxGTK31, not wxGTK30, which is used for KiCad on macOS (at least currently).
#95462 is about wxPython 4.1, not 4.0, as used here.

Anyway, KiCad requires a specially patched version of wxWidgets, at least for 3.0.4., so a stock wxWidget won't work.

The "linear" way of doing things would be either wxPython checks for the correct version of wxWidgets or KiCad gets the wxWidgets version from wxPython (if scripting for KiCad is enabled).

Do you agree that the wxWidgets versions in KiCad and wxPython should match?

At least on macOS everything here builds, I don't know yet if the scripting interface of KiCad is actually working.

Copy link
Contributor Author

@hannesweisbach hannesweisbach Sep 24, 2020

Ah. I just saw your link to (https://gitlab.com/kicad/code/kicad/-/issues/4431) in #95462. So that answers that.

then wxGTK30.override { withGtk2 = false; }
then wxGTK30.override {
withGtk2 = false;
withWebKit = true;
Copy link
Member

@evils evils Sep 24, 2020

can you clarify what withWebKit is for?

Copy link
Contributor Author

@hannesweisbach hannesweisbach Sep 24, 2020

No, just copied from

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

I took that as "wxPython_4_0 requires WebKit support", so it actually should not be anything "additional".

I think wxPython_4_0 should check if the passed-in wxGTK actually has all features enabled it needs. But I'm not sure. Or wxPython should passthru its version of wxGTK. I don't know which is more appropriate here. Probably both.

Copy link
Member

@evils evils Sep 24, 2020

i suspect the closest to wxPython checking its wxGTK(wxWidgets) dependency is "does it compile with this"
having wxPython build its specific wxWidgets and passing it through (and maybe, somehow, preferring that if both wxPython and wxGTK are passed into a package) is probably a fair solution

Copy link
Contributor Author

@hannesweisbach hannesweisbach Sep 25, 2020

So how about something like:

wxGTK = wxPython.wxwidgets;

and

assert stdenv.isDarwin -> wxGTK.version == "special-darwin-version-of-wxwidgets

in kicad/default.nix?

Copy link
Member

@evils evils Sep 28, 2020

wxGTK = wxPython.wxwidgets;

i think that will pull in wxPython even if it is otherwise not required ( for example when scriptingSupport = false; )

Copy link
Contributor Author

@hannesweisbach hannesweisbach Sep 29, 2020

"Something like" means "properly guarded" ;)

I'm currently using:

assert scriptingSupport -> stable;
[…]
python = python3;
wxPython = python.pkgs.wxPython_4_0;

wxGTK = if (scriptingSupport)
    # build Kicad with same wxWidgets as wxPython, see #98634
    then wxPython.wxGTK
    else if (stable)
    # wxGTK3x may default to withGtk2 = false, see #73145
    then wxGTK30.override { withGtk2 = false; }
    # wxGTK31 currently introduces an issue with opening the python interpreter in pcbnew
    # but brings high DPI support?
    else wxGTK31.override { withGtk2 = false; };

(#97101 changes wxPython.wxGTK to wxPython.wxwidgets.)

I don't yet if I can do: assert scriptingSupport -> stdenv.lib.versionAtLeast python3.pkgs.wxPython_4_0.wxGTK.version "3.0"

Edit: I've updated the PR in this regard. The original code clearly wasn't correct.

@evils
Copy link
Member

@evils evils commented Sep 24, 2020

btw, i'd just like to say thanks for the attention you've been giving kicad on darwin

@evils
Copy link
Member

@evils evils commented Sep 24, 2020

oh and we should probably be using wxGTK30-gtk3/wxGTK31-gtk3...

@hannesweisbach
Copy link
Contributor Author

@hannesweisbach hannesweisbach commented Sep 24, 2020

oh and we should probably be using wxGTK30-gtk3/wxGTK31-gtk3...

KiCad uses/requires a specially patched version of wxWidgets, which currently is at 3.0.4. KiCad themselves maintain a fork of wxWidgets with all patches required at https://gitlab.com/kicad/code/wxWidgets

I don't know, if newer wxWidget/KiCad versions require patches too, but I've not successfully built KiCad yet with something other than that wxWidgets version. (I don't know why you'd use a toolkit just to require platform-specific changes, nor I am sure I want to know ;))

@evils
Copy link
Member

@evils evils commented Sep 24, 2020

KiCad uses/requires a specially patched version of wxWidgets, which currently is at 3.0.4. KiCad themselves maintain a fork of wxWidgets with all patches required at https://gitlab.com/kicad/code/wxWidgets

as i understand it, kicad is (at least in linux) expected to work with upstream wxWidgets
and it indeed works with 3.0, 3.1.3, 3.1.4
(with the caveat of kicad issue 4431 which isn't resolved by using wxPython 4.1 and wxWidgets 3.1.4)

@hannesweisbach
Copy link
Contributor Author

@hannesweisbach hannesweisbach commented Sep 25, 2020

KiCad uses/requires a specially patched version of wxWidgets, which currently is at 3.0.4. KiCad themselves maintain a fork of wxWidgets with all patches required at https://gitlab.com/kicad/code/wxWidgets

I forgot an important part of the specification: on macOS :)

as i understand it, kicad is (at least in linux) expected to work with upstream wxWidgets
and it indeed works with 3.0, 3.1.3, 3.1.4
(with the caveat of kicad issue 4431 which isn't resolved by using wxPython 4.1 and wxWidgets 3.1.4)

Yep, on Linux ;)
I don't know about 3.1.x on macOS. I think I tried 3.1.3 and that didn't work for me. So I went the road of least resistance and did what kicad-mac-builder does (Yes, they have a builder for KiCad on macOS ;) (https://gitlab.com/kicad/packaging/kicad-mac-builder))

Since wxPython is used in KiCad to enable scripting via Python I think the
right thing to do is to use the same version of wxGTK for both KiCad and
wxPython.
@evils
Copy link
Member

@evils evils commented Oct 1, 2020

i think this change on its own only increases the chance of kicad not building
it'll catch a wxwidgets - wxpython mismatch at compile time
but that mismatch doesn't make kicad entirely unusable (at least for my use case)
while blocking compilation on it does...

do i understand it correctly the goal of this change is to make it easier to use wxwidgets-kicad-macos?
if so, i think it's probably best to add this change to #98538, which is where you want to use it?

@hannesweisbach
Copy link
Contributor Author

@hannesweisbach hannesweisbach commented Oct 2, 2020

Yes, you are correct. I guess it is the tradeoff between fail-early and fail-late.

If you dont want/need scripting support just disable it - the KiCad derivation will behave just as before that change. But if it is enabled it better work (imo). What else is the point of sciptingSupport, if you get a version of KiCad where scripting doesn't work when you set scriptingSupport = true?

do i understand it correctly the goal of this change is to make it easier to use wxwidgets-kicad-macos?

The point of this particular commit is making scripting work, if scriptingSupport = true. Or, at least, remove the failure mode where scripting fails because of a version mismatch between wxPython.wxGTK and the wxGTK version used to compile KiCad.
This is why scriptingSupport = false implies the old logic of selecting the wxGTK version.

Maybe the solution here is to disable scriptingSupport by default? Idk. Since it probably isn't working anyway?

if so, i think it's probably best to add this change to #98538, which is where you want to use it?

It seems to me wxPython is more picky about the wxWidgets version it gets compiled with/against, than KiCad, which is why I added it here.
Be that as it may, does it matter which derivation fails? Ah, you're using Linux (Nix-OS?), right. So if it fails inside #98538 it wouldn't affect you?

@evils
Copy link
Member

@evils evils commented Oct 2, 2020

i'm not sure if scripting support on unstable currently doesn't work, it could just be the python shell popup window that's not working
(the popup shell has been fixed with #98951)
if scripting works even without the popup shell, #88923 should work even with the current mismatch (this is untested)

my current assumption is that a runtime mismatch of wxPython and wxWidgets only breaks the GUI elements of scripting support
with the admitted bias that that is the only failure i know how to detect

It seems to me wxPython is more picky about the wxWidgets version it gets compiled with/against, than KiCad

which is why passing a quasi random wxWidgets to wxPython seems like a good way to break wxPython, and kicad along with it

but between kicad, kicad-unstable and getting either to work on darwin, and wxPython's continuing use of non-release wxWidgets
it seems to me that carefully composing wxPython in the kicad package is indeed a fair way forward

unless someone can figure out how to resolve these things in the wxPython package, then maybe you can pass wxGTK31 into wxPython, and use wxPython.wxwidgets

@hannesweisbach
Copy link
Contributor Author

@hannesweisbach hannesweisbach commented Oct 5, 2020

my current assumption is that a runtime mismatch of wxPython and wxWidgets only breaks the GUI elements of scripting support
with the admitted bias that that is the only failure i know how to detect

Oh, ok. I took failure to open the scripting console in the GUI as "scripting doesnt work". I don't know a way to test scripting either.

It seems to me wxPython is more picky about the wxWidgets version it gets compiled with/against, than KiCad

which is why passing a quasi random wxWidgets to wxPython seems like a good way to break wxPython, and kicad along with it

To me too, which is why I am not doing that, but building KiCad with the wxwidgets version of wxPython, if scripting support is enabled.

Maybe scripting should be default on, but GUI scripting should be default off, i.e the follwing CMake variables of KiCad default to OFF:

KICAD_SCRIPTING_WXPYTHON
KICAD_SCRIPTING_WXPYTHON_PHOENIX

and maybe

KICAD_SCRIPTING_ACTION_MENU

?

That would corresponding to Nix' philosophy of building to most comprehensive version of the package (i.e. scripting enabled) and keep the number of options down. At the same time it would disable (by default) potentially not compiling/not working functionality (GUI scripting). What do you think?

but between kicad, kicad-unstable and getting either to work on darwin, and wxPython's continuing use of non-release wxWidgets
it seems to me that carefully composing wxPython in the kicad package is indeed a fair way forward

Why? wxPython should specify with which wxWidgets it wants to be compiled. That has nothing to do with KiCad. I'm just using the same (version of) wxwidgets for KiCad …

If we enabled general scripting by default and introduced an option for GUI-scripting we could for now avoid the issues with wxPython.

unless someone can figure out how to resolve these things in the wxPython package, then maybe you can pass wxGTK31 into wxPython, and use wxPython.wxwidgets

Tested it and it won't compile. I didn't dig deeper into why, because it's my understanding that neither is supported by KiCad nor by wxPython (at least on macOS).
wxPython 4.1 didn't compile either (iirc).
That's the point where I basically gave up doing things on my own decided that the path of least resistnace is following the "official" builder of KiCad on macOS (https://gitlab.com/kicad/packaging/kicad-mac-builder/-/tree/master/kicad-mac-builder).

evils
evils approved these changes Oct 11, 2020
Copy link
Member

@evils evils left a comment

i look forward to using this along with #98951

@evils
Copy link
Member

@evils evils commented Oct 11, 2020

Why? wxPython should specify with which wxWidgets it wants to be compiled. That has nothing to do with KiCad. I'm just using the same (version of) wxwidgets for KiCad …

sorry, i hadn't seen the latest change and assumed you were still passing wxWidgets into wxPython

does this not lose the ability to use #98538?

@hannesweisbach
Copy link
Contributor Author

@hannesweisbach hannesweisbach commented Oct 12, 2020

Why? wxPython should specify with which wxWidgets it wants to be compiled. That has nothing to do with KiCad. I'm just using the same (version of) wxwidgets for KiCad …

sorry, i hadn't seen the latest change and assumed you were still passing wxWidgets into wxPython

Lol, ok. I was totally confused ;)

does this not lose the ability to use #98538?

Well, no. These are orthogonal, imo.

First, using the same wxwidgets for wxPython and KiCad is mandatory if you want the scripting console in pcbnew. (As a sidenote: it is my understanding now, that scripting may very well work, even if the scipting console in pcbnew does not. It is my understand that, if import pcbnew works in python, then the scripting should work. Any other tests should really be part of pcbnew/KiCad.)

Second, #98538 is used to build wxPython on macOS (since it is on macOS KiCad is really picky, at least right now). Hopefully that changes in the future, but right now that is what we have to work with :(

I imagine something like: wxPython-kicad = wxPython.override{wxGTK = wxGTK30-gtk3-kicad } either als standalone package (unlikely) or inside KiCad (kicad/default.nix):

instead of (currently) doing:

 wxPython = python.pkgs.wxPython_4_0;

I imagine something like:

 wxPython = if stdenv.isDarwin
  python.pkgs.wxPython_4_0.override{ wxGTK = wxGTK30-gtk3-kicad; }
else
  python.pkgs.wxPython_4_0;

Thus getting the desired behaviour on macOS while preserving the status quo on all other OSs.

Well, at least that's how it will work in my mind ;)

Still waiting on #96267, #98379 and #98565. It seems to me no package except KiCad builds app bundles for macOS on Nix ;)

@tfmoraes
Copy link
Contributor

@tfmoraes tfmoraes commented Nov 13, 2020

Hi @evils! #98951 was already merged.

@stale
Copy link

@stale stale bot commented Jul 21, 2021

I marked this as stale due to inactivity. → More info

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

Successfully merging this pull request may close these issues.

None yet

4 participants