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

qt6(msvc): fix xerces.dll missing when running from Qt Creator #1144

Merged
merged 13 commits into from
Jul 13, 2024

Conversation

xypp3
Copy link

@xypp3 xypp3 commented Jul 11, 2024

fix: Qt6 xerces.dll missing

TLDR

Fix issue that caused Qt6 Seamly2D xerces-c_3_2.dll not be be compiled by including it directly in the library folder.

Description

For the last couple of weeks we have been working on understanding the issues that stood in the way of Qt6. We've been in commuincation with Sue (Github username: @slspencer). This PR originates from the "qt6" branch and is intend to be merged back into it.

Problem

This PR fixes issue presented in issue#1018. The issue states that when trying to compile Seamly2D on Windows 10 with MSVC19, the build failed. Screenshot below is taken from issue page:

screeshot showing xerces.dll not found

Fix

The compiled xerces-c_3_2.dll is placed inside the src/libs/xerces/msvc folder. The .dll was taken from the Seamly xerces hosted assets

Demo

Video below shows both Qt6 version of Seamly2D compiling and being able to open files without freezing/crashing:
video demo of fixed Qt6 Seamly2D

Build Environment

TLDR: This was tested on a VM running Windows 10.

For more details on the build environment read the gist linked here

@CLAassistant
Copy link

CLAassistant commented Jul 11, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

what-the-diff bot commented Jul 11, 2024

🎉 PR Summary

👍 The hard work and dedication are definitely paying off, folks! 🚀 Here's an easy-to-digest birds-eye view of the progress in this PR:

  • Updates done in the Continuous Integration Process
    We have updated our system's 'knowledge' to a more recent version (6.5.3 as opposed to 6.5.2) to make our testing and building process faster and more secure! Inside baseball: this update was done in the .github/workflows/ci.yml file.🏗️

  • Refining Our Ignore List
    A minor adjustment in our .gitignore file is done to ensure we keep our repository clean and focused, excluding some unnecessary technical files (specifically xerces-c_3_2.dll) from our file tracking system.🗑️

  • Improving Our Code's Efficiency
    We've given some variables (like modeStr in export_layout_dialog.cpp) better default values for better performance and to avoid potential bugs. It's all about the details!🔎

  • Enhancing Robustness
    By adding qobject.h in our ifdef.cpp code file, we've broadened the abilities of our code, making it more versatile and fortified. 💪

  • Library Addition
    We have included a new file (xerces-c_3_2.dll) within src/libs/xerces-c/msvc/lib/ that contributes to the better functioning of our application. Bigger and better, folks!📚

Let's keep this amazing momentum up and continue reaching for the stars! 🌟

@slspencer slspencer self-assigned this Jul 11, 2024
@slspencer slspencer self-requested a review July 11, 2024 16:07
@slspencer slspencer removed their assignment Jul 11, 2024
Copy link
Member

@csett86 csett86 left a comment

Choose a reason for hiding this comment

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

How is the dll built, which version of xerces is used here? Please document that so that we can keep it in sync if an update to xerces is required

@@ -763,7 +763,7 @@ void ExportLayoutDialog::writeSettings() const
*/
QString ExportLayoutDialog::modeString() const
{
QString modeStr = QStringLiteral();
QString modeStr = QStringLiteral("");
Copy link
Member

Choose a reason for hiding this comment

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

Why this unrelated change here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the compiler was complaining... the current Qt 5 is using QString modeStr = QStringLiteral("");

I must have made the change sometime between the Qt6 branch and now.

Copy link
Author

Choose a reason for hiding this comment

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

I found that the change was commited on the "develop" branch on this commit

I will omit this change from the PR because it only produces a clang warning and doesn't crash the program

@@ -64,6 +64,7 @@
#include <QString>

#include "../vmisc/diagnostic.h"
#include "qobject.h"
Copy link
Member

Choose a reason for hiding this comment

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

Why this additional include?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean #include "../vmisc/diagnostic.h"?... which is already included in ifcdef.h

image

Copy link
Author

Choose a reason for hiding this comment

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

This is another place where the DRS fork and the Seamly repo diverged. This issue was fixed here. I'll change this PR to implement the fix in that PR.

@csett86
Copy link
Member

csett86 commented Jul 11, 2024

@xypp3 for more details see #1011

Copy link
Member

@csett86 csett86 left a comment

Choose a reason for hiding this comment

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

See previous comments and inline

@slspencer
Copy link
Collaborator

@DSCaskey - Would you be so kind as to enter into these comments how to fix the Linux Fails during builds on the latest GitHub runners? A bulleted list of steps to take would be beneficial.

@csett86
Copy link
Member

csett86 commented Jul 11, 2024

I can fix the windows mingw build in a separate PR.

The linux builds are fine, no need to do there anything.

And for the unit tests, they are still on the initial to do list since last May (#949), so I would suggest to tackle them also separately.

@csett86 csett86 linked an issue Jul 11, 2024 that may be closed by this pull request
@csett86
Copy link
Member

csett86 commented Jul 11, 2024

I have fixed the CI topics in #1146, please review there @slspencer or @DSCaskey

@xypp3
Copy link
Author

xypp3 commented Jul 12, 2024

@csett86

@xypp3 for more details see #1011

This is where we got the .dll from. I will include this detail in the main PR

@csett86 csett86 changed the title fix: Qt6 xerces.dll missing (issue#1018) qt6: fix xerces.dll missing when running from Qt Creator Jul 13, 2024
@csett86 csett86 changed the title qt6: fix xerces.dll missing when running from Qt Creator qt6(msvc): fix xerces.dll missing when running from Qt Creator Jul 13, 2024
@csett86 csett86 merged commit e1338cd into FashionFreedom:qt6 Jul 13, 2024
7 of 10 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.

Build: xerces error on Windows/qt6
6 participants