Skip to content

Enhance: add support for Windows 64 Bit Discord RPC Library#3200

Merged
SlySven merged 4 commits intoMudlet:developmentfrom
SlySven:enhance_addSupportForWindows64BitDiscordRPCLibrary
Dec 1, 2019
Merged

Enhance: add support for Windows 64 Bit Discord RPC Library#3200
SlySven merged 4 commits intoMudlet:developmentfrom
SlySven:enhance_addSupportForWindows64BitDiscordRPCLibrary

Conversation

@SlySven
Copy link
Copy Markdown
Member

@SlySven SlySven commented Oct 27, 2019

This entails changing the name of the existing 32 Bits one so that another one for 64 Bits can be placed alongside it in the same directory. The Discord release zip file puts them (and the Linux and MacOs 64 bit ones) in separate directories - which is a little inconvenient for us...

I have also put in some qDebug() messages - because I am having difficulty getting the Discord RPC to respond to local builds - though it works fine for Linux AppImages - which use the very same copy of the library for THAT OS. They are enabled with a DEBUG_DISCORD #define visible to the discord.cpp compilation unit...

Signed-off-by: Stephen Lyons slysven@virginmedia.com

This entails changing the name of the existing 32 Bits one so that another
one for 64 Bits can be placed alongside it in the same directory. The
Discord release zip file puts them (and the Linux and MacOs 64 bit ones) in
separate directories - which is a little inconvenient for us...

I have also put in some qDebug() messages - because I am having difficulty
getting the Discord RPC to respond to local builds - though it works fine
for Linux AppImages - which use the very same copy of the library for THAT
OS. They are enabled with a DEBUG_DISCORD #define visible to the
discord.cpp compilation unit...

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Oct 27, 2019

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Oct 27, 2019

This is small part of the work that will be needed to get a 64 Bit Windows build working - but I am having some fun debugging Discord because it isn't working for me on Linux on local builds and until I can get it working on that OS locally it makes me hesitant to work on getting it going on (my MSYS2) Windows 64 bits...

@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Oct 27, 2019

Also the prototype for the Discord_Initialise function seems to have developed an additional const char * argument for an optional steam Id - and I wasn't sure whether that was what was messing with my local builds.

for (auto& libraryPath : qApp->libraryPaths()) {
qDebug() << " " << libraryPath;
}
if (auto msg = mpLibrary->errorString(); !msg.isEmpty()) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

💯

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Wondered if you were gonna spot that C++17-ism...! 😝

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven SlySven marked this pull request as ready for review November 2, 2019 17:53
@SlySven SlySven requested a review from a team as a code owner November 2, 2019 17:53
@SlySven SlySven requested review from a team November 2, 2019 17:53
@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Nov 2, 2019

I need this now for PR #3208 - or rather the squashed down one (to eliminate all the commit noise in the git history) that I will produce once I have gotten it to the stage that it produces runnable mudlet.zip archives.

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Nov 2, 2019

It looks good, but I can't test to verify that it works, I'm on mobile. Can you get someone to test it?

Turns out the ones I had put in were both 32-bit ones - the ones inserted
here are described differently by the MSYS2 `file` command as:
* discord-rpc32.dll: "PE32 executable (DLL) (console) Intel 80386, for MS
                      Windows"
* discord-rpc64.dll: "PE32+ executable (DLL) (console) x86-64, for MS
                      Windows"
Further more they were certified and signed on "‎27 ‎November ‎2018 17:20" so
that is consistent with them being the (current) version 3.4.0 release
files from that same date.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Nov 3, 2019

It has been pulled in to #3208 and that showed I had not provided the correct library for the 64Bit case hence the need for that last commit. Feedback from the Discord #Testing channel might be soon forthcoming...

Copy link
Copy Markdown
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

I apologise for the delay in reviewing :( it works well!

@vadi2 vadi2 assigned SlySven and unassigned vadi2 Nov 29, 2019
// Defined on both 32 and 64 bit Windows
mpLibrary.reset(new QLibrary(QStringLiteral("discord-rpc32")));
#else
// All other OSes
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Well Linux and macOs - and they are both 64-Bit only as it happens.

These are not wanted in the default case for production code.
@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Nov 30, 2019

I apologise for the delay in reviewing :( it works well!

It should do as it only renames an existing file for Win32 (our current production builds) usage...

@SlySven SlySven merged commit ec08c82 into Mudlet:development Dec 1, 2019
@SlySven SlySven deleted the enhance_addSupportForWindows64BitDiscordRPCLibrary branch December 1, 2019 20:01
dicene pushed a commit to dicene/Mudlet that referenced this pull request Feb 19, 2020
)

This entails changing the name of the existing 32 Bits one so that another
one for 64 Bits can be placed alongside it in the same directory. The
Discord release zip file puts them (and the Linux and MacOs 64 bit ones) in
separate directories - which is a little inconvenient for us...

I have also put in some qDebug() messages - because I am having difficulty
getting the Discord RPC to respond to local builds - though it works fine
for Linux AppImages - which use the very same copy of the library for THAT
OS. They are enabled with a DEBUG_DISCORD #define visible to the
discord.cpp compilation unit...

Turns out the origin Windows libraries I had put in were both 32-bit ones -
the ones inserted now are described differently by the MSYS2 `file` command as:
* discord-rpc32.dll: "PE32 executable (DLL) (console) Intel 80386, for MS
                      Windows"
* discord-rpc64.dll: "PE32+ executable (DLL) (console) x86-64, for MS
                      Windows"
Further more they were certified and signed on "‎27 ‎November ‎2018 17:20" so
that is consistent with them being the (current) version 3.4.0 release
files from that same date.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven SlySven restored the enhance_addSupportForWindows64BitDiscordRPCLibrary branch June 22, 2020 18:05
@SlySven SlySven deleted the enhance_addSupportForWindows64BitDiscordRPCLibrary branch June 22, 2020 18:59
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.

2 participants