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

Deduplicate Qt plugins deployment #175

Merged
merged 2 commits into from
Aug 3, 2024

Conversation

dantti
Copy link
Contributor

@dantti dantti commented Jun 13, 2024

Sorry I added pre-commit locally to make sure clang-format was run and apparently your sources were not in sync with it

@dantti
Copy link
Contributor Author

dantti commented Jun 13, 2024

I have update with my .clang-format but still a bit different, I can manually fix it by hand :(, but I'd rather wait for your review and input about code style :)

@TheAssassin
Copy link
Member

This PR is unfortunately not acceptable because one cannot even tell what changes you implemented. You should never just run an autoformatter you like on an entire code base and the PR it.

Feel free to fix the PR and provide just the changes you want to propose. But, generally, I advise people to open issues first.

@dantti
Copy link
Contributor Author

dantti commented Jun 16, 2024

as I said I thought this project had .clang-format, it's a pain to work with projects that don't, and pre-commit just makes sure maintainers don't need to keed pointing to style issues which I have seen you pointing at on another PR.

I there was a proper style file I could just rebase and the PR would be fine.

@dantti dantti marked this pull request as draft June 18, 2024 21:19
@dantti dantti force-pushed the dantti/dedup_plugins branch 6 times, most recently from cc46109 to efacba7 Compare June 18, 2024 21:49
@dantti dantti marked this pull request as ready for review June 18, 2024 21:50
@dantti
Copy link
Contributor Author

dantti commented Jun 18, 2024

@TheAssassin done, I didn't bother to remove the using namespace .. which is not in use anymore, I can remove if you like...

@dantti
Copy link
Contributor Author

dantti commented Jun 18, 2024

Basically, most deployers had the same code that I moved into BasicPluginsDeployer::deploy() and read what was changing in each deployer with the list from qtPluginsToBeDeployed, for the corner cases where there was more logic BasicPluginsDeployer::customDeploy() is called with no need to call the base class as the code should go to BasicPluginsDeployer::deploy().

Copy link
Member

@TheAssassin TheAssassin left a comment

Choose a reason for hiding this comment

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

Not a fan of having both deploy and doDeploy (and I think the "deployStandard" name is a bit misleading), but these things tend to happen during refactorings. It's not really a problem. Looks good to me. I can work with these changes!

In the next iteration, we may just use templating to generate all the necessary deployers on the fly and further reduce the amount of code.

Please note that I just ran a quick smoke test. We need to be prepared for incoming issues.

Thank you so much!

@TheAssassin
Copy link
Member

Build failure is due to a change in the upstream runtime repo. Fixing this on master, rebasing afterwards.

@TheAssassin
Copy link
Member

TheAssassin commented Aug 2, 2024

Should be fixed. Let's wait for the results. Thanks to @dantti and all the reviewers @bjorn for reviewing. I really do enjoy the outcome.

@TheAssassin TheAssassin merged commit 0a29e02 into linuxdeploy:master Aug 3, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants