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

add include-qt-plugins #26

Merged
merged 1 commit into from Jun 9, 2023
Merged

add include-qt-plugins #26

merged 1 commit into from Jun 9, 2023

Conversation

JaegerStephan
Copy link
Contributor

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
Copy link
Member

kayhayen commented Jun 4, 2023

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 kayhayen added the enhancement New feature or request label Jun 4, 2023
@kayhayen kayhayen self-assigned this Jun 4, 2023
@kayhayen
Copy link
Member

kayhayen commented Jun 4, 2023

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
Copy link
Member

kayhayen commented Jun 4, 2023

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
Copy link
Contributor Author

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

@kayhayen
Copy link
Member

kayhayen commented Jun 6, 2023

@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
Copy link
Member

kayhayen commented Jun 6, 2023

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
Copy link
Contributor Author

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
Copy link
Member

kayhayen commented Jun 9, 2023

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

@@ -176,6 +180,12 @@ runs:
$args += @("--enable-plugin=$plugin");
}
}
if ("${{ inputs.include-qt-plugins }}" -ne ''){
Copy link
Member

Choose a reason for hiding this comment

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

This is bound to create an options usage that will not work for Nuitka 1.6 yet, but for 1.7 and current develop it will do the right thing, even if the input is a list, it will no longer override, but amend values to the plugin.

@kayhayen kayhayen merged commit c09d487 into Nuitka:main Jun 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants