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

[qwt-qt6] new port #20921

Merged
merged 1 commit into from
Nov 30, 2021
Merged

[qwt-qt6] new port #20921

merged 1 commit into from
Nov 30, 2021

Conversation

MehdiChinoune
Copy link
Contributor

@MehdiChinoune MehdiChinoune commented Oct 22, 2021

Describe the pull request

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

@ghost
Copy link

ghost commented Oct 22, 2021

CLA assistant check
All CLA requirements met.

@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Oct 22, 2021
ports/qwt/vcpkg.json Outdated Show resolved Hide resolved
ports/qwt-qt5/vcpkg.json Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

Also please sign CLA.

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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for qwt but no changes to version or port version.
-- Version: 6.2.0
-- Old SHA: e7204097fda082c43e704e356702fd77aa3c9a52
-- New SHA: fdfaffe15fff0c27dfe99119dd0ffc09630038ba
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 506a0d251720d6f5c020b0afcac751f5c6ec6c42 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 6843275..0cdc76d 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -5754,7 +5754,7 @@
     },
     "qwt": {
       "baseline": "6.2.0",
-      "port-version": 1
+      "port-version": 0
     },
     "qwt-qt5": {
       "baseline": "6.2.0",
diff --git a/versions/q-/qwt.json b/versions/q-/qwt.json
index 6b6f7a0..21451b5 100644
--- a/versions/q-/qwt.json
+++ b/versions/q-/qwt.json
@@ -1,10 +1,5 @@
 {
   "versions": [
-    {
-      "git-tree": "fdfaffe15fff0c27dfe99119dd0ffc09630038ba",
-      "version-semver": "6.2.0",
-      "port-version": 1
-    },
     {
       "git-tree": "e7204097fda082c43e704e356702fd77aa3c9a52",
       "version-semver": "6.2.0",

@MehdiChinoune
Copy link
Contributor Author

Why The CIs are building packages other than qwt and qwt-qt5?

@dg0yt
Copy link
Contributor

dg0yt commented Oct 22, 2021

Why The CIs are building packages other than qwt and qwt-qt5?

The CI builds all packages which are not found in cache. There were recent merges to master, so there can be cache misses also for unrelated packages. This is a known CI issue, and there is work in progress to improve this.

@JackBoosY
Copy link
Contributor

Please ping me if this PR is ready for review.

@MehdiChinoune
Copy link
Contributor Author

How to mark/declare conflicting packages?

@MehdiChinoune
Copy link
Contributor Author

I have added a patch to qwt-qt5 to rename the library to qwt-qt5.

@MehdiChinoune
Copy link
Contributor Author

I have just taken a look at azure pipelines, Hours of CI are wasted to build unrelated packages with every push.

Shouldn't these packages got built and cached just after their PRs got merged.

@dg0yt
Copy link
Contributor

dg0yt commented Oct 22, 2021

I have just taken a look at azure pipelines, Hours of CI are wasted to build unrelated packages with every push.

Shouldn't these packages got built and cached just after their PRs got merged.

The details are off-topic for your PR. I only want to point to microsoft/vcpkg-tool#210 now, and hope to see it in production very soon.

@JackBoosY
Copy link
Contributor

How to mark/declare conflicting packages?

if (EXISTS "${CURRENT_INSTALLED_DIR}/share/CONFLICT_PORT_NAME/copyright")
   # Conflict detected.
endif()

scripts/ci.baseline.txt Outdated Show resolved Hide resolved
@MehdiChinoune
Copy link
Contributor Author

I suspect that arm64_windows failure is related to #20933

@MehdiChinoune
Copy link
Contributor Author

MehdiChinoune commented Oct 22, 2021

qwt was not building with Qt6.
#20935 should be merged first.

Update:
I was right, doing a clean build of qwt on windows fails by not finding qmake.
Unless #20933 got fixed, this PR does change nothing.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Nov 25, 2021
ports/qwt-qt6/portfile.cmake Outdated Show resolved Hide resolved
ports/qwt-qt6/portfile.cmake Outdated Show resolved Hide resolved
@@ -1231,6 +1231,7 @@ quickfix:x64-windows-static-md=fail
quickfix:x64-windows=fail
quickfix:x86-windows=fail
qwt:x64-osx=fail
qwt-qt6:x64-osx=fail
Copy link
Member

Choose a reason for hiding this comment

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

I'm testing on a mac now...

@JackBoosY JackBoosY added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Nov 26, 2021
@BillyONeal
Copy link
Member

The osx failure is:

make[1]: *** No rule to make target `/Users/bion/vcpkg/installed/x64-osx/debug/lib/libQt6OpenGLWidgets.a', needed by `../lib/libqwt.6.2.0.dylib'.  Stop.
make: *** [sub-src-all-ordered] Error 2

I observe there is no libQt6OpenGLWidgets.a but there is installed/x64-osx/debug/lib/libQt6OpenGLWidgets_debug.a Is that ringing any bells in terms of obvious mixups?

@Neumann-A
Copy link
Contributor

Is that ringing any bells in terms of obvious mixups?

looks like it is setup the wrong way. Maybe the PRL file for libQt6OpenGLWidgets is wrong?

Co-Authored-By: Billy O'Neal <[email protected]>
@MehdiChinoune
Copy link
Contributor Author

MehdiChinoune commented Nov 26, 2021

I think the macos failure is not from the qwt side. So could you please merge this PR and resolve/discuss the issue in another PR.

@BillyONeal
Copy link
Member

The reason I’m trying to run this down is that we generally try to avoid adding new ci.baseline.txt entries unless we are doing a major bulk add (e.g. turning on a new platform or triplet) or have some understanding as to why our infrastructure can’t test a particular port.

That said, that desire isn’t absolute; I want to make sure an attempt has been made to avoid adding that entry, but it isn’t the end of the world if not.

You said you expect the build system here is reading “PRL” files?

@MehdiChinoune
Copy link
Contributor Author

Another issue!
I should wait for another 2-4 weeks to be fixed by another PR, and then I have to rebase my changes which will maybe lead to the discovery of another issue, and so on...

@JackBoosY JackBoosY added depends:different-pr This PR or Issue depends on a PR which has been filed and removed requires:author-response labels Nov 30, 2021
@JackBoosY
Copy link
Contributor

Please ping me if you continue this PR.

@MehdiChinoune
Copy link
Contributor Author

depends on #21695

@BillyONeal
Copy link
Member

Another issue! I should wait for another 2-4 weeks to be fixed by another PR, and then I have to rebase my changes which will maybe lead to the discovery of another issue, and so on...

I think the existence of #21695 provides the understanding of the issue that I was looking for, thanks!

@BillyONeal BillyONeal merged commit a295be2 into microsoft:master Nov 30, 2021
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Nov 30, 2021
@BillyONeal
Copy link
Member

Unfortunately this has caused breakage in our most recent full catalog build:

    Installing package qwt-qt6[core]:x86-windows...
    The following files are already installed in D:/installed/x86-windows and are in conflict with qwt-qt6:x86-windows

    Installed by qwt:x86-windows
        bin/qwt.dll
        debug/bin/qwtd.dll
        debug/lib/qwtd.lib
        lib/qwt.lib

    Elapsed time for package qwt-qt6:x86-windows: 34.6 s

Any chance we could convince upstream to make these side-by-side-able or should we disable one or the other in CI?

@MehdiChinoune
Copy link
Contributor Author

Unfortunately this has caused breakage in our most recent full catalog build:

    Installing package qwt-qt6[core]:x86-windows...
    The following files are already installed in D:/installed/x86-windows and are in conflict with qwt-qt6:x86-windows

    Installed by qwt:x86-windows
        bin/qwt.dll
        debug/bin/qwtd.dll
        debug/lib/qwtd.lib
        lib/qwt.lib

    Elapsed time for package qwt-qt6:x86-windows: 34.6 s

Any chance we could convince upstream to make these side-by-side-able or should we disable one or the other in CI?

Some Linux distributions rename them to qwt-qt5 and qwt-qt6 to remove the conflict. I tried to do the same but @JackBoosY refused.

@MehdiChinoune MehdiChinoune deleted the qwt-qt6 branch December 2, 2021 05:41
@JackBoosY
Copy link
Contributor

We should not rename the official library name, we should skip one of them in ci.baseline.txt.

@BillyONeal
Copy link
Member

We should not rename the official library name, we should skip one of them in ci.baseline.txt.

Under normal circumstances we should not do that, but the qt5/qt6 split here is a little different in that there appears to be no canonical names for distinguishing to match from upstream. RedHat, Debian etc. all have different ways of doing that selection.

However, given that this new port is causing failures to show up in other ports, I'm removing it again in #21825 . I discussed this with other maintainers @ras0219 / @ras0219-msft and @vicroms to figure out what a reasonable answer would be. Together we can up with the following list of things that would have to change if you want this port back @MehdiChinoune :

  • This port and the qwt one need to some how feed into the qwt build system which version of qt they are expected to build with; as far as we can tell this construction is path dependent (e.g. vcpkg install qt5-base && vcpkg install qwt-qt6 doesn't necessarily produce the same qwt bits as vcpkg install qwt-qt6 because the selection is done by guessing from previous dependencies)
  • The headers should be installed to something like qt6/qwt, so that someone can ensure they get the qt6 versions by adding that earlier on their include path. (We observe that this pattern is what Red Hat does and we like it)
  • The build system should be patched to add some kind of qt6 marker to the binary names so that they are side by side able with the qwt port.
  • (ideally, but we wouldn't block over) There should be some form of pkgconfig files which make downstream consumption reasonably easy.

Alternately, you could look at adding a qt6 version of the port to a different registry. See example public ones in #17161

We are sorry that the road has been so long on this one but we have to place a premium on not breaking folks over new things being added. It would be nice to have some kind of 'meta' port story in the future to more easily solve conditional alternative dependencies like this in the future, but we don't know exactly what that would look like at this time and don't want to ask you to do that kind of major feature development to get a port you want.

@MehdiChinoune
Copy link
Contributor Author

I have packaged qwt(-qt6) for both Homebrew and MINGW and both have a way to mark packages conflicting.
Homebrew/homebrew-core#84246
msys2/MINGW-packages#9210

I don't see adding a package to ci-baseline as a mechanism to declare conflict, a user will end up trying to install both.

@ras0219-msft
Copy link
Contributor

Our goals with vcpkg are slightly different from those systems; whereas Homebrew and MINGW both have the ultimate goal of providing applications (and libraries are a means to those ends), vcpkg's ultimate goal is to provide a coherent set of libraries for development (and applications are a means to those ends). At the end of the day, vcpkg is a C/C++ library management tool, not a Linux(-ish) system package manager even though we share a lot of common themes.

Our eventual goal is what @BillyONeal mentioned at the end:

It would be nice to have some kind of 'meta' port story in the future to more easily solve conditional alternative dependencies like this

Users should be able to easily decide whether they want to use qt5 or qt6, and then just install qwt which will use their chosen version of Qt. It would be unfortunate to duplicate every package downstream of qt into -qt5 and -qt6 variants, because a user can never use both at the same time (and indeed -- might not even care!). Plus, imagine a world with more than just qt handled this way; you'd need 2^N packages to handle the explosion of boost-w/libressl-w/qt6-w/opencv3-....

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! depends:different-pr This PR or Issue depends on a PR which has been filed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[qwt] provides qt6 version (won't fix)
7 participants