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

changed: Migrate to Qt 6.2 #941

Merged
merged 60 commits into from
May 22, 2023

Conversation

Martin-Nyaga
Copy link

@Martin-Nyaga Martin-Nyaga commented May 12, 2023

This PR updates Seamly & SeamlyMe to Qt 6.2. The changes are summarised at a high level below.

Update deprecated/changed Qt modules and APIs from 5.15 to 6.2:

The majority of code changes involve updating deprecated or changed Qt modules and APIs. With the exception of XML parsing & validation, the updates primarily consist of minor edits to includes and replacing old Qt 5 APIs and classes with their Qt 6.2 counterparts.

Add Xerces 3.2.4 for xml parsing & validation:

Since xmlpatterns is no longer available in Qt 6, this PR replaces it with Apache Xerces. The code modifications cover all areas that previously used xmlpatterns. Xerces 3.2.4 binaries and headers have been included in the extern folder. Note that the headers make up a large number of files in the PR - these are unmodified and can be skipped over if you like. Additionally, the necessary changes to .pro and .pri files have been made for linking.

Update CI builds Qt 6.2

  • Win32 is no longer supported by Qt 6.2 and has been removed from the CI pipeline.
  • We haven't updated the mac CI pipleine, as we can't easily replicate the environment. Some help on this would be appreciated.
  • There are still some failing tests for translations, which also need to be updated.

I'm tagging @hr-98 and @Niphemy from the team at DRS Software who were primarily involved in the update and can help address any questions.

cc: @slspencer

@DSCaskey
Copy link
Contributor

@hr-98

passes Windows build
I've not seen any errors about QJsonObjects when working on it, but we didn't compile it with mingw for the windows side, we used msvc.

Not for me locally... I getting all kinds of errors with QJsonObject and all kinds of link errors in vdomdocument.o. I am using mingw... it's my only choice at the moment. I can remove the parts that are using QJsonObject as it's unused code now, but I currently can't clone your branch without changes to my security settings.

@csett86

schema versions 0.6.7 and 0.6.8 are not adapted in the same way as all the other versions, thats why schema validation is failing on patterns with 0.6.7

Could you be more specific? 0.6.7 and 0.6.8 only add new attributes, so there is no conversion.

@DSCaskey
Copy link
Contributor

@slspencer

merge into develop after issues are resolved

Agreed... if I can't get this to build locally, I'm out.

@csett86
Copy link
Collaborator

csett86 commented May 17, 2023

Regarding the schema changes: The xerces requires the version specified slightly different, thats why the PR adapts all schema files slightly, see any of it in this PR. Version 0.6.7 and 0.6.8 were missed, because they were recently added when this migration was already underway. So @hr-98 can you please fix this?

@DSCaskey
Copy link
Contributor

@hr-98

Figured out the issue with QJsonObject, or at least why I'm having an issue with it and MSVC is not... it's due to conditionals for Win & GCC since the bug reporting is / was based on using DrMingw.

@DSCaskey
Copy link
Contributor

@csett86

because they were recently added when this migration was already underway

TY. That makes sense.

@hr-98
Copy link

hr-98 commented May 17, 2023

Version 0.6.7 and 0.6.8 were missed

I'll have a look at it tomorrow, can you give me patterns that use those versions? The main difference between xerces and xmlpatterns was how they handle regex patterns, xerces being a bit more strict about what's allowed.

since the bug reporting is / was based on using DrMingw

I'm not sure if I understand? What is the role of DrMingw in the build process?

@DSCaskey
Copy link
Contributor

@hr-98

I can remove the extra win32 scope, I just thought that's how it should be done because everything else seemed to be in its own separate scope.

Thanks. That's due to the fact everyone doing their own style thing over the years with no one person maintaining consistency. Since joining the project that's what I've been trying to do. I just like things clear and easy to read.

@DSCaskey
Copy link
Contributor

@Martin-Nyaga

If the Xerces build process is not too complex, we could consider adding the library as a git submodule instead, which would remove all the files from this repository and keep it cleaner. Access to the full source would allow you to build it correctly for all supported targets, and in theory make future updates more straightforward. But it does mean additional steps in CI and for development set up. What do you think?

Any reason we couldn't just move the Xerces lib source to the seamly2d/src/libs and just build it like the rest of the libs?

I would assume that would resolve the issues I'm having with undefined references to the exercise lib.

@csett86 csett86 changed the title changed: Migrated Seamly & SeamlyMe to Qt 6.2 changed: Migrate to Qt 6.2 May 17, 2023
@hr-98
Copy link

hr-98 commented May 17, 2023

Any reason we couldn't just move the Xerces lib source to the seamly2d/src/libs and just build it like the rest of the libs?

Since I am using linux when I added xerces I just used the lib from the package manager. Then when it came to making it work on Windows I wanted to mirror what I did on linux, so I compiled the library. If we added the source I don't know what I would have to do in the qmake files to make it compile and link the library, but it would make it at least work the same on all platforms.

@csett86
Copy link
Collaborator

csett86 commented May 18, 2023

Version 0.6.7 and 0.6.8 were missed

I'll have a look at it tomorrow, can you give me patterns that use those versions? The main difference between xerces and xmlpatterns was how they handle regex patterns, xerces being a bit more strict about what's allowed.

Take any pattern from https://github.com/FashionFreedom/Seamly2D/tree/develop/src/app/share/collection, eg. Steampunk_trousers.vit

and it will not open but display the following error:

Bildschirmfoto 2023-05-17 um 18 23 36

@Martin-Nyaga Martin-Nyaga changed the base branch from develop to qt6 May 18, 2023 08:28
@Martin-Nyaga
Copy link
Author

Given that this PR is currently from our fork, I think it would be a good idea to merge it into the qt6 branch so @DSCaskey and @csett86 can contribute directly to help bring this to a state where it can be merged to develop. @hr-98 has solved duplicate scopes, new pattern schema updates (0.6.7 and 0.6.8) and removed commented includes. Once we have a fix for the sound translations, is there anything else that needs to be done before we can merge at least to qt6? We can then work on the more significant outstanding issues based on that branch.

@csett86
Copy link
Collaborator

csett86 commented May 18, 2023

I dont see any blockers left, and I would tackle the CI after this is merged to qt6 in this repo

@slspencer
Copy link
Collaborator

@Martin-Nyaga Would you retarget this build to a "qt6" feature branch in ci.yml? When that's done I'll merge this PR.

@DSCaskey
Copy link
Contributor

Ok... I updated to Qt 6.5.0, and installed VS - so I should now be able to build with the Qt 6.5.0 MSVC19 64 kit. Fingers crossed.

@DSCaskey
Copy link
Contributor

Well, after installing the SDK for Windows 10, and adding umpteen paths I got it to build.. BUT it's not running. It opens with the main window messed up, and as soon as I click on anything it locks up and is non repsonsive. :(

Screenshot (1430)

@csett86
Copy link
Collaborator

csett86 commented May 22, 2023

I also tried the windows version under win10 from the artifacts (https://github.com/FashionFreedom/Seamly2D/suites/13061432105/artifacts/708310421) and that at least launched.

But opening a pattern, (eg. https://github.com/FashionFreedom/Seamly2D/blob/develop/src/app/share/collection/Steampunk_trousers.val) fails with the following nondescript error:

Bildschirmfoto 2023-05-22 um 18 46 32

Same in seamlyme for multisize measurements (eg. https://github.com/FashionFreedom/Seamly2D/blob/develop/src/app/share/tables/multisize/GOST_man_ru.vst):

Bildschirmfoto 2023-05-22 um 18 48 54

Interestingly it worked with single size measurements (eg. https://github.com/FashionFreedom/Seamly2D/blob/develop/src/app/share/collection/Steampunk_trousers.vit)

Bildschirmfoto 2023-05-22 um 18 49 26

@csett86
Copy link
Collaborator

csett86 commented May 22, 2023

We can tackle all of that in the qt6 branch once the comment from @slspencer is fixed:

@Martin-Nyaga Would you retarget this build to a "qt6" feature branch in ci.yml? When that's done I'll merge this PR.

@Martin-Nyaga
Copy link
Author

@csett86 I also downloaded the built artifact and I've been able to replicate the cryptic file loading error both in the packaged installer and in dev. So we can help take a look at these. However, I'm unable to replicate @DSCaskey's startup issue unfortunately.

I was speaking with @slspencer earlier, and it seems to me that it would be best for you to configure the CI in a way that makes sense for the project, as changing the branch in the current ci file would end up publishing releases from the qt6 branch.

@DSCaskey
Copy link
Contributor

Interestingly it worked with single size measurements (eg.

I took a cursory look at SeamlyME built with 6.5.0 and MSVC19-64, and it seems to work fine with both vit and vst files.

However, I'm unable to replicate @DSCaskey's startup issue unfortunately.

Given that ME seems to run OK I have hope. :) Other than that I'm at a loss why the 2D display is messed up... as per the previous screencap you can see the void at the far right which should be the docks. The app opens, the bell sound goes off, and the app is locked up as soon as anything is clicked on. I'll try running in debug mode and see if that presents anything as to the issue.

@csett86 csett86 merged commit 7edce9f into FashionFreedom:qt6 May 22, 2023
7 of 9 checks passed
@csett86
Copy link
Collaborator

csett86 commented May 22, 2023

Lets continue the conversation here: #949

@Martin-Nyaga
Copy link
Author

Thanks @csett86!

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.

None yet

7 participants