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: Use Cmake instead of qmake #12845

Merged
merged 1 commit into from
Sep 19, 2022

Conversation

EstebanDugueperoux2
Copy link
Contributor

Specify library name and version: qwt/6.2.0


  • [x ] I've read the guidelines for contributing.
  • [ x] I've followed the PEP8 style guides for Python code in the recipes.
  • [ x] I've used the latest Conan client version.
  • [ x] I've tried at least one configuration locally with the conan-center hook activated.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@EstebanDugueperoux2 EstebanDugueperoux2 force-pushed the package/qwt branch 2 times, most recently from 9b75114 to 05ebf53 Compare September 6, 2022 13:29
@ericLemanissier
Copy link
Contributor

I don't think it's acceptable to introduce a 6.2.1 version which actually packages 6.2.0, because official 6.2.1 will come some day. Also, there should in theory be no difference between the existing 6.2.0 binaries and the binaries built from this PR.
if @MehdiChinoune's license allows it, I think it would be best to extract the relevant CMakeLists.txt files from his repo, add them to the recipe's folder and add them to export_sources attribute.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@ghost
Copy link

ghost commented Sep 6, 2022

I detected other pull requests that are modifying qwt/all recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

Copy link
Contributor

@ericLemanissier ericLemanissier left a comment

Choose a reason for hiding this comment

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

thanks !

recipes/qwt/6.2.0/conanfile.py Outdated Show resolved Hide resolved
recipes/qwt/6.2.0/conanfile.py Outdated Show resolved Hide resolved
recipes/qwt/6.2.0/conanfile.py Outdated Show resolved Hide resolved
recipes/qwt/6.2.0/conanfile.py Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@ericLemanissier ericLemanissier left a comment

Choose a reason for hiding this comment

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

Thanks!

@EstebanDugueperoux2 EstebanDugueperoux2 marked this pull request as ready for review September 16, 2022 20:11
@EstebanDugueperoux2
Copy link
Contributor Author

@uilianries @jgsogo Could you review this PR please?
Regards.

@conan-center-bot
Copy link
Collaborator

All green in build 39 (f5917e59f880fc2eb2b8084044762030c448498e):

  • qwt/6.2.0@:
    All packages built successfully! (All logs)

@ericLemanissier
Copy link
Contributor

your force-pushed again. I don't have time to re-review a thousand lines change.

@EstebanDugueperoux2
Copy link
Contributor Author

your force-pushed again. I don't have time to re-review a thousand lines change.

It was only to have a single commit in the history instead of many draft commits.
I have don't make code changes since your last approval.

@ericLemanissier
Copy link
Contributor

https://github.com/conan-io/conan-center-index/blob/304d808ff74d68da12802fb88417a255d5de269e/docs/review_process.md#automatic-merges

The conan-center-bot will perform a squash and merge. You don't need to rebase your pull request, we ask you not to do it because it will dismiss any reviews and the reviewer will need to restart.

@EstebanDugueperoux2
Copy link
Contributor Author

https://github.com/conan-io/conan-center-index/blob/304d808ff74d68da12802fb88417a255d5de269e/docs/review_process.md#automatic-merges

The conan-center-bot will perform a squash and merge. You don't need to rebase your pull request, we ask you not to do it because it will dismiss any reviews and the reviewer will need to restart.

Ok sorry for the disturb.

@conan-center-bot conan-center-bot merged commit 1b7da89 into conan-io:master Sep 19, 2022
Comment on lines +6 to +9
"6.2.0":
- patch_file: "patches/cmake-support.patch"
patch_source: "https://github.com/MehdiChinoune/qwt/blob/cmake/CMakeLists.txt"
- patch_file: "patches/cmake-support-patch.patch"
Copy link
Contributor

@SpaceIm SpaceIm Sep 19, 2022

Choose a reason for hiding this comment

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

There are thousands lines from a fork, who will maintain that in CCI? I mean there are tones of CMakeLists we don't even care about in CCI, related to examples.
Was it submitted upstream?

If upstream is not interested, I would prefer a custom CMakeLists exported with the recipe (nobody wants to maintain big CMakeLists in patches), or go back to qmake...

Copy link
Contributor

Choose a reason for hiding this comment

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

It was proposed upstream in https://sourceforge.net/p/qwt/feature-requests/95/#397d/9909
For the reason why this patch is needed, see #12830 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

In https://sourceforge.net/p/qwt/feature-requests/95/#397d/9909, it's not clear whether these patches will be included in the near future.

Copy link
Contributor

Choose a reason for hiding this comment

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

And in patch_source, a link to a commit would be more useful.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, patch_source helps a lot on that case.

@fdgStilla
Copy link
Contributor

the error happens because qwt 6.1.6 is built with qmake, and qmake saves hardcoded path to qt dependencies during qt build. these paths are incorrect during qwt build because CCI uses a different folder for conan cache during each CI build

Hi @ericLemanissier, I created a package for qt installer framework (https://doc.qt.io/qtinstallerframework/ifw-getting-started.html#building-from-sources) and everything was fine when I was building the qt package (it needs some options disabled in conan center CI) and the qt installer framework on the same machine.

But now I am trying to create the package on different machine (because of our own CI architecture) and I am facing the issue you describe: the path of the qt dependencies are hardcoded in the qmake file.
As a specific example: lib/Qt5FontDatabaseSupport.prl reference a hardcoded path to freetype.lib

Do you have a solution to suggest? I don't think we can use another make tool to compile the qt installer framework and I do not see any workaround to fix the hardcoded paths.

@ericLemanissier
Copy link
Contributor

the (not so) quick and easy solution is to rebuild qt locally (-b qt command line argument)
the proper fix would be to patch all prl files. You could take a look at what vcpkg does: https://github.com/microsoft/vcpkg/blob/f9bea5d58186dc14e7e33132e43b52222147f51e/ports/qtbase/cmake/qt_install_submodule.cmake#L198

@fdgStilla
Copy link
Contributor

Ok thanks for the answer, like you said building qt locally is a (painful) solution but unfortunately I won't have the skill/time to patch qt.

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.

7 participants