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.

Sign up for GitHub

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

Jump to bottom

[qt5-winextras] remove unneeded dependencies #23177

Merged
merged 2 commits into from
Mar 17, 2022
Merged

[qt5-winextras] remove unneeded dependencies #23177

merged 2 commits into from
Mar 17, 2022

Conversation

Osyotr
Copy link
Contributor

@Osyotr Osyotr commented Feb 19, 2022

qt5-winextras builds fine without qt5-declarative and qt5-multimedia

cc @Neumann-A

github-actions[bot]
github-actions bot requested changes Feb 19, 2022
View reviewed changes
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 5cf60186a241e84e8232641ee973395d4fde90e1 -- versions
./vcpkg x-add-version --all
Diff 
diff --git a/versions/baseline.json b/versions/baseline.json
index 6b3147e..75f3719 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5742,7 +5742,7 @@
     },
     "qt5-winextras": {
       "baseline": "5.15.2",
-      "port-version": 1
+      "port-version": 2
     },
     "qt5-x11extras": {
       "baseline": "5.15.2",
diff --git a/versions/q-/qt5-winextras.json b/versions/q-/qt5-winextras.json
index 1d4926a..b4fb2fa 100644
--- a/versions/q-/qt5-winextras.json
+++ b/versions/q-/qt5-winextras.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "5edc85ce4269426848721ae20dbf33401f224dcb",
+      "version-string": "5.15.2",
+      "port-version": 2
+    },
     {
       "git-tree": "85a345a5fdc5a15584e6b2add00f1669e4099dbc",
       "version-string": "5.15.2",

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/qt5-winextras/vcpkg.json

Valid values for the license field can be found in the documentation

@Osyotr Osyotr force-pushed the qt5_winextras_deps branch from e95eec8 to cd77571 Compare February 19, 2022 00:26
github-actions[bot]
github-actions bot approved these changes Feb 19, 2022
View reviewed changes
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.

You have modified or added at least one vcpkg.json where a "license" field is missing.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/qt5-winextras/vcpkg.json

Valid values for the license field can be found in the documentation

@Neumann-A
Copy link
Contributor

You need to check that there is no conditional in the project files if qt5-declarative is installed or not. Otherwise the build becomes dependent on build order. The dependencies.yaml list deps but is not always correct. Do manual checking of the scripts is required. It is likly that additional qml widgets will be build if qtdeclarative can be found.

@Neumann-A
Copy link
Contributor

since it has a qtHaveModule(quick) you cannot simply remove the deps. Qtmultimedia is only used in the examples so that might be removeable.

@Osyotr
Copy link
Contributor Author

Osyotr commented Feb 19, 2022

Would it be acceptable if I introduce quick feature for this check?

@Osyotr Osyotr changed the title [qt5-winextras] remove unneded dependencies [qt5-winextras] remove unneeded dependencies Feb 19, 2022
@dg0yt
Copy link
Contributor

dg0yt commented Feb 19, 2022

I don't have the full picture right now, but I think it might be useful to have a declarative feature already at the qtbase level, and model relevant dependencies in the other parts of Qt. Apart from those users which don't need declarative, it might also significantly trim the host triplet build. (Unless there are host tools which need it.)

@Neumann-A
Copy link
Contributor

Would it be acceptable if I introduce quick feature for this check?

Yes but the project files will need patching.
Also make sure that the feature is still tested in CI by making it a requirement in the qt5 metaport

declarative feature already at the qtbase level

no. It just needs the same/similar features as qt6. The problem is that every module requires patches to its project files if it has other modules conditional stuff like qtHaveModule.

@Cheney-W Cheney-W added category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines labels Feb 21, 2022
@Neumann-A
Copy link
Contributor

@Cheney-W remove reviewed..... this is not ready to be merged.

@vicroms vicroms removed the info:reviewed Pull Request changes follow basic guidelines label Feb 23, 2022
@Cheney-W
Copy link
Contributor

Cheney-W commented Mar 4, 2022

Is this PR ready now?

@Osyotr
Copy link
Contributor Author

Osyotr commented Mar 4, 2022

@Cheney-W nope. I'll upload patches sometime later

@Cheney-W
Copy link
Contributor

@Osyotr Could you please solve the conflicts? Thank you!

@Osyotr Osyotr marked this pull request as draft March 11, 2022 09:33
@Osyotr Osyotr closed this Mar 11, 2022
@Osyotr Osyotr force-pushed the qt5_winextras_deps branch from cd77571 to e6509c9 Compare March 11, 2022 12:55
@Osyotr Osyotr reopened this Mar 11, 2022
@Osyotr Osyotr marked this pull request as ready for review March 11, 2022 12:58
Neumann-A
Neumann-A reviewed Mar 11, 2022
View reviewed changes
ports/qt5-winextras/portfile.cmake
Comment on lines +3 to +9
vcpkg_list(SET _patches
"patches/unrequire_quick.patch"
)
if("declarative" IN_LIST FEATURES)
list(APPEND _patches
"patches/require_quick.patch"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks so painful to do but there is probably no other way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My first attempt was to add requires(qtHaveModule(quick)) into qtwinextras.pro, but it skips this module instead of breaking with error.

@Osyotr
Copy link
Contributor Author

Osyotr commented Mar 11, 2022

Should I add extras (or all) to default-features of qt5 metaport so that changes from this PR are actually tested in CI?

@Osyotr
Copy link
Contributor Author

Osyotr commented Mar 14, 2022

Ping @Cheney-W for review

@Cheney-W Cheney-W added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Mar 17, 2022
@dan-shaw dan-shaw merged commit 6f827f2 into microsoft:master Mar 17, 2022
@Osyotr Osyotr deleted the qt5_winextras_deps branch March 17, 2022 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Neumann-A Neumann-A Neumann-A left review comments

@github-actions github-actions[bot] github-actions[bot] approved these changes

Assignees

@Cheney-W Cheney-W

Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Osyotr @Neumann-A @dg0yt @Cheney-W @vicroms @dan-shaw