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

Add option to enable opengl support in QCustomPlot #11591

Conversation

DonOregano
Copy link
Contributor

Specify library name and version: qcustomplot/2.1.0

This PR adds an option to enable opengl support in the QCustomPlot recipe. In the current recipe there is a TODO that this should be done, and this PR fixes that TODO. The default is to have opengl support disabled, so as to not change the behaviour for current users.

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

@CLAassistant
Copy link

CLAassistant commented Jul 7, 2022

CLA assistant check
All committers have signed the CLA.

@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@DonOregano
Copy link
Contributor Author

Something broke in the Visual Studio build. Will close and reopen this PR to trigger CI rebuild. (This is what I was told to do in another recipe I was working on).

@DonOregano
Copy link
Contributor Author

@SpaceIm, looking at the history for qcustomplot conanfile it looks like you have been involved in the creation of this recipe. Can you help me understand what is going wrong here? There is something about a missing dependency in one of the visual studio builds.

@SpaceIm
Copy link
Contributor

SpaceIm commented Jul 9, 2022

I guess it's not directly related to qcustomplot but to a new transitive dependency in qt dependency graph.

@conan-center-bot

This comment has been minimized.

@SpaceIm
Copy link
Contributor

SpaceIm commented Jul 9, 2022

/ping @jgsogo, qt5 recipe has been updated recently (#11531), and I don't see any other PR afterwards which could change its package ids, so maybe few binaries have not been uploaded to artifactory?

I see on https://conan.io/center/qt?version=5.15.3&tab=dependencies&os=Windows that several Visual Studio configurations are missing. qt recipes raises for MT & shared, so for each Visual Studio version, we should see 6 configurations (4 for static, 2 for shared), but I see only 4 package id for Visual Studio 2017 & 3 for Visual Studio 2019.

@DonOregano
Copy link
Contributor Author

Thanks for looking into this! It did seem like something out of my control...

recipes/qcustomplot/all/conanfile.py Outdated Show resolved Hide resolved
recipes/qcustomplot/all/conanfile.py Outdated Show resolved Hide resolved
recipes/qcustomplot/all/conanfile.py Outdated Show resolved Hide resolved
DonOregano and others added 2 commits July 11, 2022 10:24
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
Co-authored-by: SpaceIm <30052553+SpaceIm@users.noreply.github.com>
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

@DonOregano DonOregano marked this pull request as draft July 11, 2022 08:36
@DonOregano DonOregano marked this pull request as ready for review July 11, 2022 09:10
@conan-center-bot

This comment has been minimized.

@DonOregano DonOregano marked this pull request as draft July 11, 2022 11:52
@SpaceIm
Copy link
Contributor

SpaceIm commented Jul 11, 2022

You should check in validate() that opengl option of qt recipe is not no.

@DonOregano
Copy link
Contributor Author

DonOregano commented Jul 11, 2022

You should check in validate() that opengl option of qt recipe is not no.

Okay, I can do that. I do however have questions about how to handle opengl32 linking on windows specifically. I have been reading https://doc.qt.io/qt-6/windows-graphics.html, and from what I understand, unless the code uses QOpenGLFunctions (which QCustomPlot does not) I have to link to OpenGL32.lib on Windows to get things to work. Another alternative would be to require that opengl in Qt is desktop, which I believe would have the same result.

To summarize the Qt opengl thing I believe it works this way: If you specify "dynamic" you have to call opengl stuff through the QOpenGLFunctions class, and Qt will magically use the appropriate opengl library. If you specify "desktop" you will get linked to OpenGL32 (through QtX::OpenGL). If you specify "dynamic" you can still add linking to OpenGL32.lib explicitly, but I think this means exactly the same thing as using "desktop".

I would appreciate guidance on how to do this in the conan recipe, in a way that provides the fewest surprises to users and is maintainable...

Another solution would be to patch QCustomPlot according to this forum post https://www.qcustomplot.com/index.php/support/forum/1281, to make "dynamic" work, but that does seem a bit too invasive.

Edit: Tried to set qt:opengl to desktop, but still get linkage errors.

@DonOregano DonOregano marked this pull request as ready for review July 16, 2022 12:05
@DonOregano
Copy link
Contributor Author

Since I didn't hear anything about my comment above I chose to just improve my initial implementation to get a reaction :-D

@conan-center-bot

This comment has been minimized.

@DonOregano
Copy link
Contributor Author

@spacelm, what do you think of the current state of this PR?

@jgsogo
Copy link
Contributor

jgsogo commented Aug 1, 2022

About ERROR: Missing binary: qt/5.15.3:f32f0b09ede8c1a7d8421f05eaf42852912db16a. I can see the package is available in Artifactory with:

conan search qt/5.15.3@ --table=table.html -r=conancenter

image

The most simple explanation is a race condition: a new recipe revision of Qt was being merged into the central repository while your PR was running... Conan was able to find the recipe (it is uploaded first) but not the package (we copy/move packages one by one and it takes some time). I've triggered this PR manually again, let's see if we find the same error.

@conan-center-bot
Copy link
Collaborator

All green in build 11 (a80b85dc6126ce4710f25c8f3d37d48ef30a73cc):

  • qcustomplot/2.1.0@:
    All packages built successfully! (All logs)

    🔸 Informative: This recipe is not ready for Conan v2

    We have started the migration process to Conan v2 and exporting recipes successfully will be required in the future.
    This is just an informative note to gain awareness about the process, no need to take any action. The plan is to enforce smaller steps that are easier to fix and, eventually, this conan export step will work.
    See the recipe migration guide to know more about the changes required.

    ERROR: Error loading conanfile at '/home/conan/w/prod_cci_PR-11591/recipes/qcustomplot/all/conanfile.py': Unable to load conanfile in /home/conan/w/prod_cci_PR-11591/recipes/qcustomplot/all/conanfile.py
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/home/conan/w/prod_cci_PR-11591/recipes/qcustomplot/all/conanfile.py", line 1, in <module>
        from conans import ConanFile, CMake, tools
    ImportError: cannot import name 'ConanFile' from 'conans' (/opt/pyenv/versions/3.7.13/lib/python3.7/site-packages/conans/__init__.py)
    
  • qcustomplot/1.3.2@:
    All packages built successfully! (All logs)

    🔸 Informative: This recipe is not ready for Conan v2

    We have started the migration process to Conan v2 and exporting recipes successfully will be required in the future.
    This is just an informative note to gain awareness about the process, no need to take any action. The plan is to enforce smaller steps that are easier to fix and, eventually, this conan export step will work.
    See the recipe migration guide to know more about the changes required.

    ERROR: Error loading conanfile at '/home/conan/w/prod_cci_PR-11591/recipes/qcustomplot/all/conanfile.py': Unable to load conanfile in /home/conan/w/prod_cci_PR-11591/recipes/qcustomplot/all/conanfile.py
      File "<frozen importlib._bootstrap_external>", line 728, in exec_module
      File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
      File "/home/conan/w/prod_cci_PR-11591/recipes/qcustomplot/all/conanfile.py", line 1, in <module>
        from conans import ConanFile, CMake, tools
    ImportError: cannot import name 'ConanFile' from 'conans' (/opt/pyenv/versions/3.7.13/lib/python3.7/site-packages/conans/__init__.py)
    

@jgsogo
Copy link
Contributor

jgsogo commented Aug 2, 2022

👍 It looks like that was the issue. Now build has passed. Time for reviews!

Thanks!

@conan-center-bot conan-center-bot merged commit 9a63989 into conan-io:master Aug 9, 2022
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.

None yet

6 participants