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

REFAC(client): Get rid of launcher and DLL on Windows #6246

Merged

Conversation

davidebeatrici
Copy link
Member

No description provided.

@davidebeatrici
Copy link
Member Author

mumble/src/mumble/main.cpp

Lines 135 to 139 in c21a90a

#if defined(Q_OS_WIN) && !defined(MUMBLEAPP_DLL)
extern "C" __declspec(dllexport) int main(int argc, char **argv) {
#else
int main(int argc, char **argv) {
#endif

I wonder what this is for, I would've expected the logic to be the opposite.

Comment on lines +1137 to +1138
set(overlay ON)
set(plugins ON)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is appropriate here 🤔
These should be defaulted where they are declared and then not overwritten just because we're packaging. I realize that this is what we had in the old code, but I'm not sure this is something we should keep 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe they are forced because the installer code doesn't take into account whether the DLLs exist.

Copy link
Member

Choose a reason for hiding this comment

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

It does for the overlay (at least there is a parameter) and maybe it would be best if it also did that for the plugins as well 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed, let's do it in a separate pull request.

Copy link
Member

@Hartmnt Hartmnt left a comment

Choose a reason for hiding this comment

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

This needs to be updated, too https://github.com/mumble-voip/mumble/blob/master/docs/dev/TheMumbleSourceCode.md#source-tree-layout

Other than that I have found not apparent issue with the changes. However, I am not qualified to substantially comment on Windows build processes :)

@davidebeatrici davidebeatrici force-pushed the windows-no-more-dll-launcher branch from 2171981 to f70fef8 Compare October 25, 2023 00:08
@davidebeatrici
Copy link
Member Author

davidebeatrici commented Oct 25, 2023

Thank you.

As a bonus we also got rid of a typo (direcrtory).

As for the exported main(): #4429

@Krzmbrzl
Copy link
Member

Krzmbrzl commented Nov 4, 2023

💚 All backports created successfully

Status Branch Result
1.5.x

Questions ?

Please refer to the Backport tool documentation and see the Github Action logs for details

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

Successfully merging this pull request may close these issues.

3 participants