pkgconfig variables support in CMake toolchain file #17101

nthirtyone · 2022-05-29T20:46:01Z

Drafted as per discussion in #17096.

I don't have Windows build environment to see how it works there and I'm unsure to what extent this is covered by the tests. Any input is appreciated.

sbc100

lgtm.

I think maybe we should add a test though, there are a few .pc files that get installed in the sysroot by default:

./cache/sysroot/lib/pkgconfig
├── egl.pc
├── glesv2.pc
└── sdl.pc

See tests/cmake for the current cmake tests.. maybe one of these can be modified/updated.

sbc100 · 2022-05-29T21:49:01Z

cmake/Modules/Platform/Emscripten.cmake

+  cmake_path(CONVERT "${_em_pkgconfig_libdir}" TO_NATIVE_PATH_LIST _em_pkgconfig_libdir)
+endif()
+set(ENV{PKG_CONFIG_LIBDIR} "${_em_pkgconfig_libdir}")
+set(ENV{PKG_CONFIG_PATH} "$ENV{EM_PKG_CONFIG_PATH}") # Stops caller's PKG_CONFIG_PATH from propagating.


I think we should forget about EM_PKG_CONFIG_PATH. I'm not sure why use it ever had.

If somebody explicitly sets PKG_CONFIG_PATH when building an emscripten project I don't see why should ignore it. That seems like a deliberate act.

This behaviour was based on tools.building.get_building_env().

I agree that it seems a bit out of place, but that would introduce inconsistency in how caller's variables are used across tools. If it's acceptable for you then I'm happy to do it.

Or should I update get_building_env? For example: create a backwards-compatible behaviour that prioritises EM_PKG_CONFIG_PATH but also allows for PKG_CONFIG_PATH, and then implement it consistently inget_building_env and here in toolchain.

I think you should ignore EM_PKG_CONFIG_PATH in this patch and we should try to remove it from get_building_env as separate change.

Alright, removed the line in question.

nthirtyone · 2022-05-30T22:00:45Z

@sbc100, Newly added test seems to work fine after adding pkg-config to Windows and Mac CI environments (as of d439aa8). It checks whether the LIBDIR variable is propagated and set to expected value and if expected packages are visible from CMake's FindPkgConfig (without verifying version to minimise the impact of any upgrades and reduce duplication - I want to add a modversion check for test in #17100).

That being said, I see that test_minimal_runtime_code_size_* are failing because "overall generated code size was improved by 43 bytes." Quick glance at the test made me confused and I cannot reproduce the results locally. Comments there suggest that it might be unrelated, but can't tell just yet.

sbc100 · 2022-05-31T16:06:37Z

tests/cmake/find_pkg_config/CMakeLists.txt

+
+message(STATUS "PKG_CONFIG_LIBDIR: $ENV{PKG_CONFIG_LIBDIR}")
+
+find_package(PkgConfig REQUIRED)


How does this file on machines that don't have pkg-config.

If might be nice to tell the person running the test exactly what is wrong (imagine a windows developer running this test for example).

Fails and drops following into stderr:

CMake Error at C:/ProgramData/chocolatey/lib/cmake.portable/tools/cmake-3.23.2-windows-x86_64/share/cmake-3.23/Modules/FindPackageHandleStandardArgs.cmake:230 (message): Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE)

Which could be confusing. Let me add a skip branch that checks whether pkg-config is available in the PATH.

I'd prefer and explict fail.. so that folks know to install it. Something like if not_in_path: self.fail('pkg-config required to run this test'.

If there are folks who are not able to don't want to install it we can consider adding an EMTEST_SKIP_PKK_CONFIG setting that will allow them to skip over it.. but hopefully it won't be needed.

(otherwise the test silently skips on new installations).

Alright, made it to fail in bfc2f5a. Since I also increased verbosity in the CMake when looking for pkg-config, we could alternatively go with the same approach as test_cmake_with_embind_cpp11_mode has: if WINDOWS and not utils.which('pkg-config'): self.skipTest and on other platforms it would go through this stage and if it wouldn't find pkg-config it would fail due to CMake's non-zero code along with a "Could not find pkg-config executable!" message in stderr.

sbc100 · 2022-05-31T16:07:24Z

tests/cmake/find_pkg_config/CMakeLists.txt

+
+find_package(PkgConfig REQUIRED)
+
+pkg_check_modules(EGL REQUIRED egl)


Maybe add a comment here: Emscripten ships .pc files for egl/glesv2/sdl. Check that they can all be located correctly

Added a message explaining what the script is trying to do. I reworded it a bit to not contain the list of packages, so that adding a new package won't require modifying the message on top of everything else. I guess we could just go with a full-pledged list and then iterate, create some nice message and so on, but I feel like that'd be an overkill for a test and I'd rather have it dumb simple.

sbc100

lgtm! (with a couple comments). Thanks for working on this.

sbc100 · 2022-05-31T16:08:41Z

The code size test failures are unrelated.. its something that needs adjusting periodically if there are change to llvm. I'll submit a fix for that and you will need to rebase/merge.

Also rename the existing `require_node` and `require_v8` decorators. See #17101

sbc100 · 2022-05-31T17:31:16Z

cmake/Modules/Platform/Emscripten.cmake

+else()
+  cmake_path(CONVERT "${_em_pkgconfig_libdir}" TO_NATIVE_PATH_LIST _em_pkgconfig_libdir)
+endif()
+set(ENV{PKG_CONFIG_LIBDIR} "${_em_pkgconfig_libdir}")


I wonder if we should also set PKG_CONFIG_SYSROOT_DIR here? (doesn't have to be part of this change).

I'd suggest to leave it out for now. We'd want to get consistent behaviour for CMake and building.py and PKG_CONFIG_SYSROOT_DIR can easily become a journey on its own (LIBDIR is good enough for most use cases, and SYSROOT affects only the PATH, not LIBDIR).

Also rename the existing `require_node` and `require_v8` decorators. See #17101

Drafted pkgconfig variables support in CMake toolchain file

5c1f039

sbc100 reviewed May 29, 2022

View reviewed changes

nthirtyone added 5 commits May 30, 2022 19:59

Added test for pc files finding via CMake

cc7b0f7

Removed {EM_,}PKG_CONFIG_PATH from toolchain

8bc8ec2

Fixed missing redirection from cmake run

6e8ae1c

Added pkgconfiglite to choco packages for windows tests

f3a4d51

Added pkg-config to Mac CI test environment

d439aa8

sbc100 reviewed May 31, 2022

View reviewed changes

sbc100 approved these changes May 31, 2022

View reviewed changes

nthirtyone added 3 commits May 31, 2022 18:37

pkg-config test will now be skipped if no pkg-config

86b81fa

Increased verbosity of CMake output for the pkg-config test

15f7812

Test will now fail instead of skipping in case of missing pkg-config

bfc2f5a

sbc100 added a commit that referenced this pull request May 31, 2022

tests: Add @requires_ninja decorator. NFC

d24e0f2

Also rename the existing `require_node` and `require_v8` decorators. See #17101

sbc100 mentioned this pull request May 31, 2022

tests: Add @requires_ninja decorator. NFC #17110

Merged

sbc100 reviewed May 31, 2022

View reviewed changes

Merge remote-tracking branch 'origin/main' into pc-with-cmake

e80be51

sbc100 enabled auto-merge (squash) May 31, 2022 20:14

sbc100 merged commit 3b36c26 into emscripten-core:main May 31, 2022

sbc100 added a commit that referenced this pull request Jun 1, 2022

tests: Add @requires_ninja decorator. NFC

61883c6

Also rename the existing `require_node` and `require_v8` decorators. See #17101

sbc100 added a commit that referenced this pull request Jun 1, 2022

tests: Add @requires_ninja decorator. NFC

9e9cb14

Also rename the existing `require_node` and `require_v8` decorators. See #17101

sbc100 added a commit that referenced this pull request Jun 1, 2022

tests: Add @requires_ninja decorator. NFC (#17110)

5312576

Also rename the existing `require_node` and `require_v8` decorators. See #17101


		message(STATUS "PKG_CONFIG_LIBDIR: $ENV{PKG_CONFIG_LIBDIR}")

		find_package(PkgConfig REQUIRED)


		find_package(PkgConfig REQUIRED)

		pkg_check_modules(EGL REQUIRED egl)

pkgconfig variables support in CMake toolchain file #17101

pkgconfig variables support in CMake toolchain file #17101

Uh oh!

Conversation

nthirtyone commented May 29, 2022

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nthirtyone commented May 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nthirtyone May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 commented May 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nthirtyone May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

nthirtyone commented May 30, 2022 •

edited

Loading

nthirtyone May 31, 2022 •

edited

Loading

nthirtyone May 31, 2022 •

edited

Loading