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

[qtkeychain] switch to Qt6; add qtkeychain-qt5; update to 0.13.1 #20185

Closed
wants to merge 2 commits into from

Conversation

Be-ing
Copy link
Contributor

@Be-ing Be-ing commented Sep 16, 2021

and update to 0.13.1

  • What does your PR fix?

    Fixes qtkeychain unable to build with Qt6. Also add libsecret dependency on Linux.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    yes

@Be-ing Be-ing changed the title [qtkeychain] add options to build with either Qt5 or Qt6 [qtkeychain] add features to build with either Qt5 or Qt6 Sep 16, 2021
@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 16, 2021

Originally I planned to fail the port build if neither qt5 nor qt6 feature was selected and intentionally not select one by default because accidentally pulling in either could result in a big increase in build time. CI is not happy about this because CI doesn't pick either, so I guess one or the other needs to be picked as a default feature. I am leaning towards picking the newer one and leaving users who need to stay with Qt5 to explicitly opt to do so. However this could surprise users who are already using this port with Qt5. Thoughts?

@JackBoosY
Copy link
Contributor

cc @Neumann-A for review this PR.

@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Sep 16, 2021
@cenit
Copy link
Contributor

cenit commented Sep 16, 2021

exclusive features are forbidden by policy (i know there exist some, but they were allowed before the policy was enforced)

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 16, 2021

What is the alternative then? Use separate packages for building QtKeychain with Qt5 or Qt6?

@Be-ing
Copy link
Contributor Author

Be-ing commented Sep 16, 2021

I got some curious errors building this with Qt6 on Windows:

-- Configuring done
CMake Error at C:/mixxx-vcpkg/installed/x64-windows/share/Qt6LinguistTools/Qt6LinguistToolsMacros.cmake:94 (add_custom_command):
  OUTPUT path

    C:/mixxx-vcpkg/buildtrees/qtkeychain/src/v0.12.0-c92b7c53b3.clean/translations/qtkeychain_de.ts

  in a source directory as an output of custom command.
Call Stack (most recent call first):
  CMakeLists.txt:111 (qt6_create_translation)
  CMakeLists.txt:210 (qt_create_translation)


CMake Error at C:/mixxx-vcpkg/installed/x64-windows/share/Qt6LinguistTools/Qt6LinguistToolsMacros.cmake:94 (add_custom_command):
  OUTPUT path

    C:/mixxx-vcpkg/buildtrees/qtkeychain/src/v0.12.0-c92b7c53b3.clean/translations/qtkeychain_fr.ts

  in a source directory as an output of custom command.
Call Stack (most recent call first):
  CMakeLists.txt:111 (qt6_create_translation)
  CMakeLists.txt:210 (qt_create_translation)


CMake Error at C:/mixxx-vcpkg/installed/x64-windows/share/Qt6LinguistTools/Qt6LinguistToolsMacros.cmake:94 (add_custom_command):
  OUTPUT path

    C:/mixxx-vcpkg/buildtrees/qtkeychain/src/v0.12.0-c92b7c53b3.clean/translations/qtkeychain_ro.ts

  in a source directory as an output of custom command.
Call Stack (most recent call first):
  CMakeLists.txt:111 (qt6_create_translation)
  CMakeLists.txt:210 (qt_create_translation)


CMake Error at C:/mixxx-vcpkg/installed/x64-windows/share/Qt6LinguistTools/Qt6LinguistToolsMacros.cmake:94 (add_custom_command):
  OUTPUT path

    C:/mixxx-vcpkg/buildtrees/qtkeychain/src/v0.12.0-c92b7c53b3.clean/translations/qtkeychain_ru.ts

  in a source directory as an output of custom command.
Call Stack (most recent call first):
  CMakeLists.txt:111 (qt6_create_translation)
  CMakeLists.txt:210 (qt_create_translation)


CMake Error at C:/mixxx-vcpkg/installed/x64-windows/share/Qt6LinguistTools/Qt6LinguistToolsMacros.cmake:94 (add_custom_command):
  OUTPUT path

    C:/mixxx-vcpkg/buildtrees/qtkeychain/src/v0.12.0-c92b7c53b3.clean/translations/qtkeychain_zh.ts

  in a source directory as an output of custom command.
Call Stack (most recent call first):
  CMakeLists.txt:111 (qt6_create_translation)
  CMakeLists.txt:210 (qt_create_translation)


-- Generating done
CMake Error:
  Running

   'C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/Common7/IDE/CommonExtensions/Microsoft/CMake/Ninja/ninja.exe' '-C' 'C:/mixxx-vcpkg/buildtrees/qtkeychain/x64-windows-dbg' '-t' 'recompact'

  failed with:

   ninja: error: loading 'build.ninja': The system cannot find the file specified.

These errors occurred in this GitHub Actions job. The logs are available as a GitHub Actions artifact.

@cenit
Copy link
Contributor

cenit commented Sep 21, 2021

What is the alternative then? Use separate packages for building QtKeychain with Qt5 or Qt6?

yes, i think so

@BillyONeal @ras0219-msft

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 13, 2021

Ping @BillyONeal and @ras0219-msft, should the new package be for Qt5 or Qt6? I think it would make sense to keep the existing package with Qt5, but the generic qt packages have been switched to Qt6 so it could go either way. 🤷 I just need an answer to proceed.

@Be-ing Be-ing mentioned this pull request Oct 13, 2021
3 tasks
@JackBoosY
Copy link
Contributor

@Be-ing Please use qt6.

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 14, 2021

That does not answer my question. Which package name should use Qt6?

@JackBoosY
Copy link
Contributor

That does not answer my question. Which package name should use Qt6?

According to our policy, we should not to add the conflict features, so please keep this port name since port qt is qt6 in the master branch.
The qt5 version should be named as qt5-keychain.

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 14, 2021

I think qt5-qtkeychain is a better package name because qtkeychain is not part of Qt.

@Be-ing Be-ing changed the title [qtkeychain] add features to build with either Qt5 or Qt6 [qtkeychain] switch to Qt6; add qt5-qtkeychain package Oct 14, 2021
@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 14, 2021

On further thought, I think qtkeychain-qt5 is a better name to distinguish it from packages that are part of Qt.

@Be-ing Be-ing changed the title [qtkeychain] switch to Qt6; add qt5-qtkeychain package [qtkeychain] switch to Qt6; add qtkeychain-qt5 package Oct 14, 2021
@JackBoosY
Copy link
Contributor

On further thought, I think qtkeychain-qt5 is a better name to distinguish it from packages that are part of Qt.

Accepted.

@Be-ing
Copy link
Contributor Author

Be-ing commented Oct 14, 2021

CI seems to be down for unrelated reasons.

@JackBoosY
Copy link
Contributor

Will rerun this PR after #20730 merge.

@microsoft microsoft locked as resolved and limited conversation to collaborators Nov 12, 2021
@microsoft microsoft unlocked this conversation Nov 12, 2021
@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 12, 2021

Well, this is weird. Since adding "host": true for the qttools dependency, CMake fails to find Qt6 on Windows. So I tried adding qtbase as a dependency without "host": true and now CMake finds Qt6 but then that brings back the error about not finding Qt Linguist. I have no idea what's going on.

@Be-ing Be-ing force-pushed the qtkeychain_qt6 branch 2 times, most recently from 13fe130 to 124abfa Compare November 12, 2021 21:08
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 356918c4c16c4ef67b6f869a852eb60bd438bcf6 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/q-/qtkeychain.json b/versions/q-/qtkeychain.json
index b6f6b59..d64613f 100644
--- a/versions/q-/qtkeychain.json
+++ b/versions/q-/qtkeychain.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "8362147ae2e80b848447dc2f88ad96965fe790c9",
+      "git-tree": "463e5f361e6d29d2ac477813e32872f63b6889c5",
       "version": "0.13.1",
       "port-version": 0
     },

and add libsecret as a dependency for Linux
@Be-ing
Copy link
Contributor Author

Be-ing commented Nov 12, 2021

CI is passing now skipping building translations for arm64-windows. I don't do any builds for arm64-windows and my users aren't asking for arm64-windows builds so I don't mind this. The libsecret build option is working for Linux. If anyone else wants to work on building qtkeychain for Qt6 with translated error messages on arm64-windows, go ahead, but I am tired of working on this one little PR and would like to move on.

@ras0219-msft
Copy link
Contributor

Thanks @Be-ing for the work to make this happen and sorry that we haven't given it enough attention to push things through.

In contrast to @JackBoosY's suggestion at #20185 (comment), we absolutely should not pull the rug out from underneath existing users that expect qtkeychain to be based on qt5. In an extreme case, we could consider removing the name qtkeychain entirely and providing qtkeychain-qt5 and qtkeychain-qt6, but I don't see a compelling reason for this extreme.

Therefore, I've opened #21420 that cherry-picks the qtkeychain commit from this PR but swaps them (back to the original arrangement?).

Thanks once again; I hope we can get this merged soon.

@cenit cenit mentioned this pull request Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants