search

Nuitka / Nuitka-Action

Action to build with Nuitka on GitHub in your workflows
MIT License
111 stars 22 forks source link

add include-qt-plugins #26

Closed JaegerStephan closed 1 year ago

JaegerStephan commented 1 year ago

Hi!

It's my first pull request. I hope I'm doing well.

I implemented the include-qt-plugins which is related to #15 and used it successfully for compiling on windows runner. I strongly aligned the code to enable-plugins.

I would ask you to test the implementation (best with linux oder macos) and approve the pull request.

Bests, Stephan

kayhayen commented 1 year ago

You are doing well. By none of your faults, the Qt plugin option will override its value entirely, rather than expanding it. So the looping is wrong. Instead the input list should be condensed to a single use of --include-qt-plugins.

I need to think a bit, if we want to have it that way. Commonly, people fall into the trap of thinking, that they do not replace the default of sensible when adding and removing stuff. In that way, maybe a change in Nuitka could make your code correct, but then we would --no-include-qt-plugins to remove from the list of sensible plugins.

Since no other option is like this, it's kind of indicative of a bug in Nuitka. I would ask you to allow for me to make a change to this option for say 1.6.2 in behavior. You would then just do the same as you did, but with --no as well, as it would be better. I do not think, we should not cater to this kind of UI bug in Nuitka, effectively making it harder to change.

kayhayen commented 1 year ago

One thing that would be nice, if these options that pertain to plugins, could be namespaced in some way, and make sure the plugin is actually enabled. I think I can take that on, once I find the time. The ordering in the schema is kind of weird for now as well.

kayhayen commented 1 year ago

A simpler change would be to just make an store=append option in Nuitka, I might even do that for 1.6.1 because that will make them additive already, even if they replace still sensible. Then subtraction remains impossible for now, unless not specifying sensible yourself, but that would be OK.

JaegerStephan commented 1 year ago

I can follow your thoughts just partially. Is there anything I should/could do?

kayhayen commented 1 year ago

@JaegerStephan no, I am just going to make code's your assumption that you can specify --include-qt-plugins multiple times work, right now it's not the case. We missed the boat for 1.6.2, but I am going it right now, the small version, that is just going to be incrememtal rather than replacing, and then we can merge as is.

kayhayen commented 1 year ago

You can run your instance of the action against current factory version of Nuitka. I hope the action docs are sufficient for you to know how to do it. :-)

JaegerStephan commented 1 year ago

Thank your for your proposal but I think I'll wait until it is on the main branch since I'm working in enterprise environment and have to request for any action (as well as new branches and versions). So far I'll work with my own fork. But I'll be happy to use it soon from a new version :-)

kayhayen commented 1 year ago

Ok, going to merge it even before 1.7, so it can be used with develop already, once my change hits there.