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

[libqglviewer] update to 2.9.1 + Qt6 + CMake #30420

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

MehdiChinoune
Copy link
Contributor

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 25, 2023

Does this package sufficiently forward link libaries for static usage via pkg-config and cmake config?

@MehdiChinoune
Copy link
Contributor Author

Does this package sufficiently forward link libaries for static usage via pkg-config and cmake config?

I haven't understand the question.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 25, 2023

Upstream builds a shared QGLViewer library. On Linux etc. users only need to link to this shared library. Dependencies are resolved at runtime.
This PR enables building a static library. When linking to a static library, users must also also link to the libraries which are required by that static library. I.e. transitive dependencies must be known at user project build time. Even the order of libraries matters. Here, we are talking about qtbase libs and their transitive usage requirements.
This information is expected to be carried in .pc files and .cmake config files. Given that upstream doesn't expect to build a static lib, I expect that the information is incomplete.

@MehdiChinoune
Copy link
Contributor Author

Upstream builds a shared QGLViewer library. On Linux etc. users only need to link to this shared library. Dependencies are resolved at runtime. This PR enables building a static library. When linking to a static library, users must also also link to the libraries which are required by that static library. I.e. transitive dependencies must be known at user project build time. Even the order of libraries matters. Here, we are talking about qtbase libs and their transitive usage requirements. This information is expected to be carried in .pc files and .cmake config files. Given that upstream doesn't expect to build a static lib, I expect that the information is incomplete.

I have disabled shared library on Windows, because It fails to build with MSVC.
Apparently they don't test/support Windows or MSVC!

@dg0yt
Copy link
Contributor

dg0yt commented Mar 25, 2023

I'm concerned about static linkage on non-windows, not about shared linkage on windows.

@MehdiChinoune
Copy link
Contributor Author

I'm concerned about static linkage on non-windows, not about shared linkage on windows.

Two options:

  • Use qmake
  • Default to static on Windows and shared elsewhere.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 25, 2023

Use qmake downstream? This is a dead end with Qt6.

The first thing is to look into the installed config files.

  • If they are complete, no problem.
  • If they can be fixed easily, fix them. (CMake should be easy to get right. But IIRC Qt6 lacks .pc files for static builds.)
  • If they cannot be fixed easily, do not offer static linkage.
    I'm not sure if the situation for static linkage is really better with Windows.
    What is the problem with dynamic linkage on Windows? It is officially supported, and I spotted at least one export macro.

@MehdiChinoune
Copy link
Contributor Author

What is the problem with dynamic linkage on Windows? It is officially supported, and I spotted at least one export macro.

Maybe they build only with MINGW, but with MSVC I got this:

...
D:\dev\vcpkg-qt5\buildtrees\libqglviewer\x64-windows-dbg\QGLViewer_autogen\SLZDDFIPUJ/moc_camera.cpp(404): error C2491: 'qglviewer::Camera::staticMetaObject': definition of dllimport static data member not allowed
...
D:\dev\vcpkg-qt5\buildtrees\libqglviewer\x64-windows-dbg\QGLViewer_autogen\SLZDDFIPUJ/moc_frame.cpp(91): error C2491: 'qglviewer::Frame::staticMetaObject': definition of dllimport static data member not allowed
...
D:\dev\vcpkg-qt5\buildtrees\libqglviewer\x64-windows-dbg\QGLViewer_autogen\SLZDDFIPUJ/moc_keyFrameInterpolator.cpp(194): error C2491: 'qglviewer::KeyFrameInterpolator::staticMetaObject': definition of dllimport static data member not allowed
...

MINGW+GCC accepts that, but MSVC (I think also MINGW+Clang) throws an error.

@dg0yt
Copy link
Contributor

dg0yt commented Mar 25, 2023

The CMake build system is new. AFAICS it misses the definition of CREATE_QGLVIEWER_DLL when building the DLL. .vcproj and .pro handle it.
https://github.com/search?q=user%3AGillesDebunne+CREATE_QGLVIEWER_DLL&type=code

@MehdiChinoune MehdiChinoune marked this pull request as ready for review March 25, 2023 14:27
@LilyWangLL LilyWangLL added the category:port-update The issue is with a library, which is requesting update new revision label Mar 27, 2023
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Mar 27, 2023
@vicroms vicroms merged commit 9659713 into microsoft:master Mar 28, 2023
Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Mar 28, 2023
[libqglviewer] update to 2.9.1 + Qt6 + CMake (microsoft#30420)
@MehdiChinoune MehdiChinoune deleted the libqglviewer-update branch March 29, 2023 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants