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

[postbuildlint] Add checks to ensure that the pkgconfig files are at the right location #171

Merged
merged 3 commits into from
Oct 11, 2021

Conversation

autoantwort
Copy link
Contributor

These checks would have stopped the regression introduced by microsoft/vcpkg#19272 reported in microsoft/vcpkg#19701 (and fixed in microsoft/vcpkg#19703)

@JackBoosY JackBoosY removed their assignment Aug 23, 2021
@Neumann-A
Copy link
Contributor

Hmm short summary what is expected and what would error?

@autoantwort
Copy link
Contributor Author

Here is the error log of what the output when I try to install the broken yaml-cpp port:

➜  vcpkg git:(master) vcpkg install --triplet=x64-osx --host-triplet=x64-osx --binarysource=clear
Detecting compiler hash for triplet x64-osx...
The following packages will be built and installed:
    yaml-cpp[core]:x64-osx -> 0.7.0
Starting package 1/1: yaml-cpp:x64-osx
Building package yaml-cpp[core]:x64-osx...
-- Downloading https://github.com/jbeder/yaml-cpp/archive/0579ae3d976091d7d664aa9d2527e0d0cff25763.tar.gz -> jbeder-yaml-cpp-0579ae3d976091d7d664aa9d2527e0d0cff25763.tar.gz...
-- Cleaning sources at /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/buildtrees/yaml-cpp/src/d0cff25763-31738b4ebd.clean. Use --editable to skip cleaning for the packages you specify.
-- Extracting source /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/downloads/jbeder-yaml-cpp-0579ae3d976091d7d664aa9d2527e0d0cff25763.tar.gz
-- Using source at /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/buildtrees/yaml-cpp/src/d0cff25763-31738b4ebd.clean
-- Found external ninja('1.10.2').
-- Configuring x64-osx-dbg
-- Configuring x64-osx-rel
-- Building x64-osx-dbg
-- Building x64-osx-rel
-- Installing: /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/packages/yaml-cpp_x64-osx/share/yaml-cpp/copyright
-- Performing post-build validation
There should be no pkgconfig directories outside of lib and debug/lib.
The following misplaced pkgconfig directories were found:

    /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/packages/yaml-cpp_x64-osx/share/pkgconfig

You can move the pkgconfig files with the following commands:

    file(INSTALL "${CURRENT_PACKAGES_DIR}/a/path/pkgconfig/name.pc" DESTINATION "${CURRENT_PACKAGES_DIR}/a/path/pkgconfig")
    file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/a/path/pkgconfig")

There should be no cmake directory in /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/packages/yaml-cpp_x64-osx/share with targets and configs files.
Fix the cmake targets and configs files with (you need a host dependency to the `vcpkg-cmake-config` port):

    vcpkg_cmake_config_fixup(CONFIG_PATH share/cmake/${PORT})

Found 2 error(s). Please correct the portfile:
    /Users/leanderSchulten/git_projekte/Lichtsteuerung/vcpkg/ports/yaml-cpp/portfile.cmake
-- Performing post-build validation done
Error: Building package yaml-cpp:x64-osx failed with: POST_BUILD_CHECKS_FAILED
Please ensure you're using the latest portfiles with `./vcpkg update`, then
submit an issue at https://github.com/Microsoft/vcpkg/issues including:
  Package: yaml-cpp:x64-osx
  Vcpkg version: 2021-08-12-unknownhash-debug

Additionally, attach any relevant sections from the log files above.

@Neumann-A
Copy link
Contributor

Neumann-A commented Aug 25, 2021

Here is the error log of what the output when I try to install the broken yaml-cpp port:

The question was more like: Which paths are allowed and which paths would error for .pc and .cmake files.

Also share/cmake/<package> is totally fine and your current check probably breaks Qt5.
The same can be said about /share/pkgconfig. pc files should only be needed in /lib/pkgconfig/ when they contain library information, otherwise they are totally fine in a common place. Think about header only packages which need to set Cflags but have no libraries attached! (or pc files for just tools)

@autoantwort
Copy link
Contributor Author

The question was more like: Which paths are allowed and which paths would error for .pc and .cmake files.

for cmake: no cmake folders anywhere
for pkgconfig: only in /lib and /debug/lib

Also share/cmake/<package> is totally fine and your current check probably breaks Qt5.

Yeah qt5 installs to /share/cmake/<package>. Is there a reason why it installs not to /share/<package>?
Do you have another idea how to check for not fixed cmake files?

The same can be said about /share/pkgconfig. pc files should only be needed in /lib/pkgconfig/ when they contain library information, otherwise they are totally fine in a common place. Think about header only packages which need to set Cflags but have no libraries attached! (or pc files for just tools)

Maybe require them at /lib when the port produces a file in the /lib folder and otherwise skip the check

@autoantwort
Copy link
Contributor Author

Do you have another idea how to check for not fixed cmake files?

Maybe only error when there is a *-targets-release.cmake file in the cmake folder and not a *-targets-debug.cmake?

@Neumann-A
Copy link
Contributor

Do you have another idea how to check for not fixed cmake files?

The point is not all cmake files need to be fixed. The reason cmake files needs fixing is due to naked find_library calls and the fact that vcpkg moves them probably from lib/cmake/<package> to share/<package> which breaks the import_prefix cmake calculated. If you have a project which already install into share/<package> or share/cmake/<package> and uses targets for everything only the debug files need to be moved. No general fixup needed.

Maybe only error when there is a *-targets-release.cmake file in the cmake folder and not a *-targets-debug.cmake?

breaks single config builds.
Within the default CI you would rather detected installed -targets-debug.cmake in the wrong directory.

@autoantwort
Copy link
Contributor Author

Ok will remove the cmake check. Maybe add a policy that allows "unusual" *.pc file locations? Because afaik currently it is an error with a probability of 90% .

@strega-nil-ms
Copy link
Contributor

strega-nil-ms commented Sep 27, 2021

Are there any current ports that install .pc files in share? We support installing pkgconfig files in share, in vcpkg_fixup_pkgconfig, so this would be a change.

@Neumann-A
Copy link
Contributor

Are there any current ports that install .pc files in share?

#9966 had one. In general pc files in share could be installed by any header only library which just needs the include path.

@strega-nil-ms
Copy link
Contributor

Then probably we want to allow .pc files in share/pkg-config as well.

@autoantwort
Copy link
Contributor Author

Then probably we want to allow .pc files in share/pkg-config as well.

Ok, we allow files in share/pkg-config when they don't have a line starting with Libs:

@strega-nil-ms
Copy link
Contributor

@autoantwort or Libs.private

@autoantwort
Copy link
Contributor Author

Done. Now also searches for *.pc files instead of pkgconfig folders because I have found *.pc files outside of pkgconfig folders.

@autoantwort
Copy link
Contributor Author

We can now also remove the Policy VCPKG_POLICY_SKIP_PKGCONFIG_LOCATION_CHECK again I think?

@autoantwort autoantwort force-pushed the add-postbuild-checks branch 3 times, most recently from fc357e1 to 4e38cb0 Compare September 30, 2021 02:17
@autoantwort autoantwort changed the title [postbuildlint] Add checks to ensure that the pkgconfig and cmake files are at the right location [postbuildlint] Add checks to ensure that the pkgconfig files are at the right location Oct 1, 2021
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Requesting changes only to the code comment, the lambda suggestion is an optional nitpick.

src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved
src/vcpkg/postbuildlint.cpp Outdated Show resolved Hide resolved
@BillyONeal BillyONeal merged commit bf4ec07 into microsoft:main Oct 11, 2021
@BillyONeal
Copy link
Member

Thanks for the new post-build check. Going to be "fun" running this on the whole tree.... :D

@BillyONeal
Copy link
Member

I think in the future before merging things like this we need to run against the full tree because blocking shipping a new tool for weeks while we deal with the fallout isn't really OK :/

@autoantwort
Copy link
Contributor Author

Yeah that's true.

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.

6 participants