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

[vcpkg_fixup_pkgconfig] Check for more problems, add unit test #23898

Merged
merged 31 commits into from
Jan 20, 2023

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented Mar 31, 2022

  • What does your PR fix?

    Adds a unit test for vcpkg_fixup_pkgconfig.
    Improves vcpkg_fixup_pkgconfig:

    • Handle line continuations.

    • Replaces ';' with ' '.

    • Resolves CMake linking keywords optimized, debug, general (from select_library_configurations)

    • Check for invalid Libs content:

      • <namespace>::<target> (from <Pkg>_LIBRARIES)
      • ...-NOTFOUND (from find_library)

      Failed checks are reported as warning, not error, to avoid problems with old port versions in manifest mode.

  • Which triplets are supported/not supported? Have you updated the CI baseline?

    all, no

  • Does your PR follow the maintainer guide?

    yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    no

@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Mar 31, 2022
@JackBoosY
Copy link
Contributor

cc @Neumann-A

@dg0yt
Copy link
Contributor Author

dg0yt commented Mar 31, 2022

FTR It is only a draft now, to get an idea of the size of the problem.
I'm undecided whether to fix individual portfiles, or to implement fixup in vcpkg_fixup_pkgconfig.

What already popped up is NOTFOUND. In some case it could be just dropped, in other cases it might indicate an actual error.

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

Updated 2022-07-03:

allegro5:
    Libs.private: -loptimized -lpng -ldebug -lpng16d -loptimized -lz -ldebug -lz -lm

arrow:
  Libs.private: SNAPPY_LIB-NOTFOUND RE2_LIB-NOTFOUND optimized;D:/installed/x64-windows/lib/bz2.lib;debug;D:/installed/x64-windows/debug/lib/bz2d.lib

dlib:
  Libs: -L${libdir} -ldlib ws2_32 winmm comctl32 gdi32 imm32 optimized  D:/installed/x64-windows/lib/libpng16.lib debug  D:/installed/x64-windows/debug/lib/libpng16d.lib optimized D:/installed/x64-windows/lib/zlib.lib debug D:/installed/x64-windows/debug/lib/zlibd.lib optimized D:/installed/x64-windows/lib/jpeg.lib debug D:/installed/x64-windows/debug/lib/jpeg.lib D:/installed/x64-windows/lib/openblas.lib D:/installed/x64-windows/lib/lapack.lib D:/installed/x64-windows/lib/openblas.lib unofficial::sqlite3::sqlite3 FFTW3::fftw3

fluidsynth:
  'Libs' refer to a CMake target: -lPkgConfig::GLIB

libconfuse:
  Libs: -L${libdir} -lconfuse optimized;D:/installed/x64-windows/lib/intl.lib;debug;D:/installed/x64-windows/debug/lib/intl.lib;D:/installed/x64-windows/lib/iconv.lib;D:/installed/x64-windows/lib/charset.lib

libgd:
  Libs.private: optimized;D:/installed/x64-windows/lib/webp.lib;debug;D:/installed/x64-windows/debug/lib/webpd.lib;optimized;D:/installed/x64-windows/lib/webpdecoder.lib;debug;D:/installed/x64-windows/debug/lib/webpdecoderd.lib;optimized;D:/installed/x64-windows/lib/webpdemux.lib;debug;D:/installed/x64-windows/debug/lib/webpdemuxd.lib;optimized;D:/installed/x64-windows/lib/libwebpmux.lib;debug;D:/installed/x64-windows/debug/lib/libwebpmuxd.lib

libgta:
  Libs.private: -loptimized -lD:/installed/x64-windows/lib/lzma.lib -ldebug -lD:/installed/x64-windows/debug/lib/lzma.lib -loptimized -lD:/installed/x64-windows/lib/bz2.lib -ldebug -lD:/installed/x64-windows/debug/lib/bz2d.lib -loptimized -lD:/installed/x64-windows/lib/zlib.lib -ldebug -lD:/installed/x64-windows/debug/lib/zlibd.lib

libpcap:
  Libs.private: -lws2_32 -loptimized -lD:/installed/x64-windows/lib/libssl.lib -ldebug -lD:/installed/x64-windows/debug/lib/libssl.lib -loptimized -lD:/installed/x64-windows/lib/libcrypto.lib -ldebug -lD:/installed/x64-windows/debug/lib/libcrypto.lib

mlpack:
  Libs: -Loptimized -LD:/installed/x64-windows/lib/boost_serialization-vc140-mt.lib -Ldebug -LD:/installed/x64-windows/debug/lib/boost_serialization-vc140-mt-gd.lib -larmadillo -loptimized -lD:/installed/x64-windows/lib/boost_serialization-vc140-mt.lib -ldebug -lD:/installed/x64-windows/debug/lib/boost_serialization-vc140-mt-gd.lib -LD:/packages/mlpack_x64-windows/lib/ -lmlpack

mongo-c-driver:
  Libs: -L${libdir} -lmongoc-static-1.0 -lsecur32.lib -lcrypt32.lib -lShlwapi.lib -lsecur32.lib -lcrypt32.lib -lBcrypt.lib -lDnsapi -loptimized -LD:/installed/x64-windows-static/lib -lzlib -ldebug -LD:/installed/x64-windows-static/debug/lib -lzlibd

sdl2cpp:
  'Libs' refer to a CMake target: SDL2::SDL2main

Errror classes:

  • ; (from CMake lists) instead of for separating command line arguments
  • optimized/debug CMake linking keywords
    • -loptimized -lrel -ldebug -ldbg (allegro5, libgta, libpcap, mlpack,mongo-c-driver)
    • -Loptimized -Lrel -Ldebug -Ldbg (mlpack)
    • optimized;librel.a;debug;libdbg.a[;optimized;...] (arrow, libconfuse, libgd)
    • optimized librel.a debug libdbg.a (dlib)
  • <Lib>-NOTFOUND (arrow)
  • namespaced::target (dlib, fluidsynth, sdl2cpp)

@Neumann-A
Copy link
Contributor

Seems like this could be fixed with REGEX MATCH ? at least the list like stuff and the -ldebug -l<lib> .... The rest probably needs manual updating.

@Neumann-A
Copy link
Contributor

Also a check for targets might be appropiate: -lOpenSSL::SSL

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

Also a check for targets might be appropiate: -lOpenSSL::SSL

Basically I need a vcpg_validate_pkgconfig as much as a vcpg_validate_pkgconfig. I don't even care for symbols at the moment, just validate the linking command...

@dg0yt
Copy link
Contributor Author

dg0yt commented Apr 1, 2022

BTW, I do have the cmake code to expand the Libs from the target properties. An early version is here in port curl, and an extended version is in GDAL. (Covering common generator expressions, too.) Maybe I should wrap that as a CMake package in a vcpkg port, and patch it in where needed.

github-actions[bot]
github-actions bot previously approved these changes Jul 1, 2022
github-actions[bot]
github-actions bot previously approved these changes Jul 3, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • scripts/test_ports/unit-test-cmake/vcpkg.json

Valid values for the license field can be found in the documentation

@dg0yt dg0yt force-pushed the fixup-pkgconfig branch from 2cbea99 to 062b870 Compare July 23, 2022 15:45
@dg0yt dg0yt changed the title [vcpkg_fixup_pkgconfig] Check for 'optimized' and 'debug' in pc files [vcpkg_fixup_pkgconfig] Check for more problems, add unit test Jul 23, 2022
@dg0yt
Copy link
Contributor Author

dg0yt commented Nov 19, 2022

unit-test-cmake: Must no longer expect errors.

qtwebengine: Strange!

-- Configuring x64-windows-dbg
-- Configuring x64-windows-rel
-- Building x64-windows-dbg
CMake Error at scripts/cmake/vcpkg_execute_build_process.cmake:131 (message):
    Command failed: D:/downloads/tools/cmake-3.24.0-windows/cmake-3.24.0-windows-i386/bin/cmake.exe --build . --config Debug --target install -- -v -j33
    Working Directory: D:/buildtrees/tmp/x64-windows-dbg
    See logs for more information:
      D:\buildtrees\tmp\install-x64-windows-dbg-out.log

But this log file is not in the failure logs archive. There is also no trace of the config step. I guess it is due to

CMake Warning at ports/qtwebengine/portfile.cmake:83 (message):
  Buildtree path 'D:/buildtrees/qtwebengine' is too long.

  Consider passing --x-buildtrees-root=<shortpath> to vcpkg!

  Trying to use 'D:/buildtrees/qtwebengine/../tmp'

not considered during CI log collection.

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 1, 2022

Ping @LilyWangLL

@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Dec 5, 2022
@lukester1975
Copy link

Nice, been waiting for this! Would you consider fixing up .private fields when generating shared libs? Whilst they are not relevant it would be nice if they weren't bad. E.g. arrow generates:

Libs.private: ${prefix}/bin/snappy.dll optimized;${prefix}/lib/bz2.lib;debug;${prefix}/debug/lib/bz2d.lib

Or maybe they should be just stripped out completely in a shared build's pc files?

Thanks!

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 8, 2022

Or maybe they should be just stripped out completely in a shared build's pc files?

This might be a nice-to-have option for vcpkg, under the assumption that vcpkg should never use static libs from outside.
However, I think it is a topic for another PR.

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 15, 2022

Ping for merge or new feedback.
CC @BillyONeal

@dg0yt
Copy link
Contributor Author

dg0yt commented Dec 18, 2022

The port is ready to merge: Only known baseline regression, no new regressions from this PR's changes.

@LilyWangLL LilyWangLL added the depends:different-pr This PR or Issue depends on a PR which has been filed label Dec 19, 2022
@LilyWangLL
Copy link
Contributor

Depends on: #28352

@dan-shaw dan-shaw removed the info:reviewed Pull Request changes follow basic guidelines label Dec 22, 2022
@LilyWangLL LilyWangLL removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Jan 5, 2023
@dan-shaw dan-shaw added the info:reviewed Pull Request changes follow basic guidelines label Jan 19, 2023
@BillyONeal BillyONeal merged commit edcf949 into microsoft:master Jan 20, 2023
@BillyONeal
Copy link
Member

Thank you for:

  • Adding tests
  • Thinking about old port versions
  • Dealing with this for 10 months

<3

@dg0yt dg0yt deleted the fixup-pkgconfig branch January 20, 2023 06:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants