Skip to content

[qt5-base] no _debug lib suffixes on macOS (#14225) #21695

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.

Sign up for GitHub

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

Merged
BillyONeal merged 9 commits into from
Dec 16, 2021
Merged

[qt5-base] no _debug lib suffixes on macOS (#14225) #21695

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
87a1c35
[qt5-base] no _debug lib suffixes on macOS (#14225)
autoantwort Nov 27, 2021
586bdff
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal Nov 30, 2021
cb7d456
[qt5-base] patch qt5-base to remove _debug postfix on osx
autoantwort Nov 30, 2021
55a3b9c
Apply suggestions from code review
autoantwort Dec 3, 2021
c42f145
[qt5-base] create link to _debug.pc file from .pc file without _debug
autoantwort Dec 5, 2021
66c197a
Merge branch 'master' into qt5-base-no-absolute2
autoantwort Dec 7, 2021
d44174d
update version
autoantwort Dec 7, 2021
40525d1
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal Dec 10, 2021
33211b6
[qt5-base] create forward pkgconfig file
autoantwort Dec 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions ports/qt5-base/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -506,6 +506,13 @@ endif()

vcpkg_fixup_pkgconfig()

if(VCPKG_TARGET_IS_OSX)
file(GLOB _debug_files "${CURRENT_PACKAGES_DIR}/debug/lib/pkgconfig/*_debug.pc")
foreach(_file ${_debug_files})
string(REGEX REPLACE "_debug\.pc$" ".pc" _new_filename "${_file}")
file(CREATE_LINK "${_file}" "${_new_filename}")

@BillyONeal BillyONeal Dec 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
file(CREATE_LINK "${_file}" "${_new_filename}")

Symlinks do not survive binary caching. It may make sense to create a new pc file that just requires the other one?

@autoantwort autoantwort Dec 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It is a hard link...

@BillyONeal BillyONeal Dec 10, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OK, that survives binary caching but still seems like it may cause a lot of duplicate settings from pkgconfig to leak into others?

@autoantwort autoantwort Dec 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What do you mean?

@ras0219-msft ras0219-msft Dec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

More specifically on whether or not to have the file: I am unaware of any other case where the debug mode .pc file would have a different name than the release build .pc file. In general this seems extremely suspect and I would prefer to avoid any possible downstream issues coming from sometimes having this .pc and sometimes missing it.

If we can find a consumer that actually expects _debug.pc files separate from the normal .pc files, then I'd be more amenable to consider this nonstandard behavior.

@autoantwort autoantwort Dec 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok, then I will simply rename the file instead of creating a copy

@autoantwort autoantwort Dec 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we can find a consumer that actually expects _debug.pc files separate from the normal .pc files, then I'd be more amenable to consider this nonstandard behavior.

All other Qt modules expect that:

-- Fixing pkgconfig file: /Users/vagrant/Data/packages/qt5-networkauth_x64-osx/lib/pkgconfig/Qt5NetworkAuth.pc
CMake Error at scripts/cmake/vcpkg_fixup_pkgconfig.cmake:78 (message):
  /usr/local/bin/pkg-config --exists Qt5NetworkAuth failed with error code: 1

      ENV{PKG_CONFIG_PATH}: "/Users/vagrant/Data/installed/x64-osx/debug/lib/pkgconfig:/Users/vagrant/Data/installed/x64-osx/share/pkgconfig:/Users/vagrant/Data/packages/qt5-networkauth_x64-osx/debug/lib/pkgconfig:/Users/vagrant/Data/packages/qt5-networkauth_x64-osx/share/pkgconfig:/Users/vagrant/Data/packages/qt5-networkauth_x64-osx/lib/pkgconfig:/Users/vagrant/Data/packages/qt5-networkauth_x64-osx/share/pkgconfig:/Users/vagrant/Data/installed/x64-osx/lib/pkgconfig:/Users/vagrant/Data/installed/x64-osx/share/pkgconfig"
      output: Package Qt5Core_debug was not found in the pkg-config search path.

  Perhaps you should add the directory containing `Qt5Core_debug.pc'

  to the PKG_CONFIG_PATH environment variable

  Package 'Qt5Core_debug', required by 'Qt5Network', not found

@Neumann-A Neumann-A Dec 10, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

All other Qt modules expect that:

I think you need to find the pkg_config_output_name in the pri files somewhere or also patch Requires: for the qt pkg-config files. The Qt5*_debug.pc stuff should never be used by downstream consumers. (and is probably also not expected.)

@ras0219-msft ras0219-msft Dec 10, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, if Qt is wanting it internally then I can see it being more trouble than value to try to patch it all the way out. Instead, I think we should then always provide both .pc filenames.

I think it would also be better to create the "alternate" using pkg-config's existing mechanisms to forward instead of symlinks/copying. Something like:

Name: Qt5NetworkAuth
Version: <copied from other file>
Requires: Qt5NetworkAuth_debug

It should be easy to just string match the "Version:" line from the existing file and include it in the forwarder.

@autoantwort autoantwort Dec 13, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have added this forwarding pkgconfig file now.

endforeach()
endif()
# #Code to get generated CMake files from CI
# file(RENAME "${CURRENT_PACKAGES_DIR}/share/cmake/Qt5Core/Qt5CoreConfig.cmake" "${CURRENT_BUILDTREES_DIR}/Qt5CoreConfig.cmake.log")
# file(GLOB_RECURSE CMAKE_GUI_FILES "${CURRENT_PACKAGES_DIR}/share/cmake/Qt5Gui/*.cmake" )
Expand Down
2 changes: 1 addition & 1 deletion ports/qt5-base/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "qt5-base",
"version-semver": "5.15.2",
"port-version": 17,
"port-version": 18,
"description": "Qt5 Application Framework Base Module. Includes Core, GUI, Widgets, Networking, SQL, Concurrent and other essential qt components.",
"homepage": "https://www.qt.io/",
"dependencies": [
Expand Down
2 changes: 1 addition & 1 deletion versions/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5526,7 +5526,7 @@
},
"qt5-base": {
"baseline": "5.15.2",
"port-version": 17
"port-version": 18
},
"qt5-canvas3d": {
"baseline": "0",
Expand Down
5 changes: 5 additions & 0 deletions versions/q-/qt5-base.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "61c4e933f6d8c618dc42f06f2cf270c7647e4a46",
"version-semver": "5.15.2",
"port-version": 18
},
{
"git-tree": "f8fbecca6183036771dc794859432a2a84435abb",
"version-semver": "5.15.2",
Expand Down