Skip to content

qwt: switch to qt6#84246

Closed
MehdiChinoune wants to merge 1 commit intoHomebrew:masterfrom
MehdiChinoune:qwt-qt6
Closed

qwt: switch to qt6#84246
MehdiChinoune wants to merge 1 commit intoHomebrew:masterfrom
MehdiChinoune:qwt-qt6

Conversation

@MehdiChinoune
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@MehdiChinoune MehdiChinoune changed the title Qwt qt6 qwt-qt6 Aug 30, 2021
@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Aug 30, 2021
@carlocab
Copy link
Member

Please split your commits so that each commit modifies only one formula:

  • One formula per commit; one commit per formula.
  • Keep merge commits out of the pull request.

When you do so, please format your commit messages according to Homebrew/core style:

At Homebrew, we like to put the name of the formula up front like so: foobar 7.3 (new formula).

The preferred commit message format for simple version updates is foobar 7.3 and for fixes is foobar: fix flibble matrix..

See the commit style guide referenced in the PR template for more information.

@carlocab carlocab added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Aug 30, 2021
@carlocab carlocab changed the title qwt-qt6 qwt: switch to qt6 Aug 30, 2021
Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Does qwtpolar not need to be switched to qt from qt@5 as well?

@BrewTestBot BrewTestBot removed the automerge-skip `brew pr-automerge` will skip this pull request label Aug 30, 2021
@MehdiChinoune
Copy link
Contributor Author

Does qwtpolar not need to be switched to qt from qt@5 as well?

qwtpolar is now part of qwt (6.2.0)

@MehdiChinoune
Copy link
Contributor Author

#{Formula["qt"].opt_include} reports an incorrect value I/opt/homebrew/opt/qt/lib/QtGui.framework/Versions/5/Headers, it should be I/opt/homebrew/opt/qt/lib/QtGui.framework/Versions/6/Headers

@carlocab
Copy link
Member

carlocab commented Aug 30, 2021

#{Formula["qt"].opt_include} reports an incorrect value I/opt/homebrew/opt/qt/lib/QtGui.framework/Versions/5/Headers, it should be I/opt/homebrew/opt/qt/lib/QtGui.framework/Versions/6/Headers

Formula["qt"].opt_include evaluates to $HOMEBREW_PREFIX/opt/qt/include, so there is nothing incorrect about the value it reports.

@carlocab
Copy link
Member

Does qwtpolar not need to be switched to qt from qt@5 as well?

qwtpolar is now part of qwt (6.2.0)

Then we need to disable it and point users to qwt.

@MehdiChinoune
Copy link
Contributor Author

MehdiChinoune commented Aug 30, 2021

Does qwtpolar not need to be switched to qt from qt@5 as well?

qwtpolar is now part of qwt (6.2.0)

Then we need to disable it and point users to qwt.

It is already disabled.

@carlocab
Copy link
Member

You also need these changes in gnuradio:

diff --git a/Formula/gnuradio.rb b/Formula/gnuradio.rb
index 88121be3e4..812e09aad9 100644
--- a/Formula/gnuradio.rb
+++ b/Formula/gnuradio.rb
@@ -103,8 +103,8 @@ class Gnuradio < Formula
       -DENABLE_DEFAULT=OFF
       -DPYTHON_EXECUTABLE=#{venv_root}/bin/python
       -DPYTHON_VERSION_MAJOR=3
-      -DQWT_LIBRARIES=#{Formula["qwt"].lib}/qwt.framework/qwt
-      -DQWT_INCLUDE_DIRS=#{Formula["qwt"].lib}/qwt.framework/Headers
+      -DQWT_LIBRARIES=#{Formula["qwt-qt5"].lib}/qwt.framework/qwt
+      -DQWT_INCLUDE_DIRS=#{Formula["qwt-qt5"].lib}/qwt.framework/Headers
       -DCMAKE_PREFIX_PATH=#{Formula["qt@5"].opt_lib}
       -DQT_BINARY_DIR=#{Formula["qt@5"].opt_bin}
       -DENABLE_TESTING=OFF

@MehdiChinoune
Copy link
Contributor Author

MehdiChinoune commented Aug 30, 2021

gnuradio couldn't be build with qwt 6.2.0
gnuradio/gnuradio#4956

@scpeters scpeters mentioned this pull request Aug 30, 2021
6 tasks
@scpeters
Copy link
Contributor

I believe any formulae in 3rd-party taps that depend on qwt will be broken when this is merged. To simplify their migration path, I've added an alias for qwt-qt5 formula in #84275 so that 3rd-party taps can change their dependencies to point to qwt-qt5 in advance. They may still be broken when that is merged, but I think there's a chance it will go more smoothly.

@carlocab carlocab added the CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. label Aug 31, 2021
@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Aug 31, 2021
@MehdiChinoune
Copy link
Contributor Author

  • Could this pr merged with broken gnuradio?
  • Should I backport the fix to gnuradio?
  • Should I wait until new release from gnuradio that supports qwt 6.2.0?

@carlocab
Copy link
Member

We don't want to merge this with gnuradio broken, but we can backport a fix if one is available.

@MehdiChinoune MehdiChinoune marked this pull request as ready for review September 2, 2021 04:00
@github-actions
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Sep 24, 2021
@MehdiChinoune MehdiChinoune reopened this Oct 11, 2021
@carlocab carlocab removed the stale No recent activity label Oct 11, 2021
@MehdiChinoune
Copy link
Contributor Author

Is there a way to silent that error about qwt-qt5 patch

==> brew audit qwt-qt5 --online --new-formula
==> FAILED
==> Installing 'bundler' gem
qwt-qt5:
  * Formulae should not require patches to build. Patches should be submitted and accepted upstream first.
Error: 1 problem in 1 formula detected

qwt-qt5 formula is a duplicate of the old qwt!

@carlocab
Copy link
Member

carlocab commented Oct 11, 2021

Unfortunately not. It is inconvenient, though, because it means the dependent tests are skipped.

It might be better to split switching qwt to Qt6 to a separate PR so that its dependents are tested properly. In the meantime we can have a qwt-qt5 (plus formulae that depend on it) that's actually just a duplicate of qwt.

@MehdiChinoune
Copy link
Contributor Author

Unfortunately not. It is inconvenient, though, because it means the dependent tests are skipped.

It might be better to split switching qwt to Qt6 to a separate PR so that its dependents are tested properly. In the meantime we can have a qwt-qt5 (plus formulae that depend on it) that's actually just a duplicate of qwt.

see #87092

@carlocab
Copy link
Member

Great; can you drop those three commits from this one?

@carlocab carlocab mentioned this pull request Oct 11, 2021
6 tasks
@MehdiChinoune
Copy link
Contributor Author

Great; can you drop those three commits from this one?

I will rebase it after the other pull-request get merged.

@carlocab
Copy link
Member

We'll want to run the tests on qwt built with Qt6 before merging #87092, but those tests won't be run if you don't drop the qwt-qt5 commits.

@BrewTestBot BrewTestBot removed the automerge-skip `brew pr-automerge` will skip this pull request label Oct 11, 2021
@carlocab
Copy link
Member

Ah, right. Forgot about the mixed dependency audit. Sigh, ok. We'll have to wait then.

Copy link
Member

@carlocab carlocab left a comment

Choose a reason for hiding this comment

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

Thanks!

@BrewTestBot
Copy link
Contributor

🤖 A scheduled task has triggered a merge.

@MehdiChinoune MehdiChinoune deleted the qwt-qt6 branch October 12, 2021 11:43
@github-actions github-actions bot added the outdated PR was locked due to age label Nov 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. outdated PR was locked due to age

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants