[libsystemd] add new port #31150

jbbjarnason · 2023-04-27T12:10:08Z

dg0yt

I think it wasn't easy to get so far. Please bear with me if my comments ask for more work. It is opinon, not an official request.

ports/libcap/cross-compile.patch

dg0yt · 2023-04-27T17:34:03Z

ports/libcap/portfile.cmake

+  FETCH_REF "libcap-${VERSION}"
+  REF 3c7dda330bd9a154bb5b878d31fd591e4951fe17
+  PATCHES
+    configure.patch


I think it might might be more useful to not patch but copy a small configure script from port to source dir which generates a minimal Makefile.vcpkg.
This makefile would have the per-config settings as needed (--prefix) and add the missing rules and dependencies (install; libcap.a vs. libcap.so etc).
In that way, you would also solve a problem with this PR: These ports will fail in release-only builds.

I assume you would ask for an example... In port openssl, there is a configure wrapper, but it doesn't have to generate a Makefile. In https://github.com/microsoft/vcpkg/pull/30608/files, there is the example with generating a Makefile, but it is work in progress.

This would require some work, I can maybe try to add it later

Two questions, what is the benefit with your proposed method? Would a if/else of VCPKG_BUILD_TYPE in portfile not be enough to fix the release only builds (this also applies to libsystemd)?

btw, I made the patch because vcpkg has an existing bug with projects that do not come with any configuration files, which you seem to know of #14389 (comment) 😃

I have changed it from being a patch to an empty configure script. However I have not made it as a general configure script since it is empty and therefore would instead like to do the required things in CMake.

ports/libsystemd/Config.cmake.in

dg0yt · 2023-04-27T17:38:49Z

ports/libsystemd/portfile.cmake

+
+vcpkg_fixup_pkgconfig()
+
+configure_file("${CMAKE_CURRENT_LIST_DIR}/Config.cmake.in" "${CURRENT_PACKAGES_DIR}/share/unofficial-systemd/unofficial-systemd-config.cmake" @ONLY)


IMO this CMake config should be removed. There is an official pc file, and the CMake config would need to rely on pkg-config for some dependencies.

In my experience, the systemd pkgconfig file is not enough they do not declare requirements since they manually make the pkgconfig file instead of utilizing the meson generator.
https://github.com/systemd/systemd/blob/main/src/libsystemd/libsystemd.pc.in

ports/libcap/configure.patch

Adela0814 · 2023-05-04T09:52:01Z

ports/libcap/portfile.cmake

+vcpkg_build_make(SUBPATH libcap
+  BUILD_TARGET cap_names.h
+  OPTIONS
+    prefix=${CURRENT_INSTALLED_DIR}
+    CC=gcc # use host architecture to generate cap_names.h
+)
+
+if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
+  vcpkg_build_make(SUBPATH libcap
+    BUILD_TARGET libcap.a
+    OPTIONS
+      prefix=${CURRENT_INSTALLED_DIR}
+  )
+else()
+  vcpkg_build_make(SUBPATH libcap
+    BUILD_TARGET libcap.so
+    OPTIONS
+      prefix=${CURRENT_INSTALLED_DIR}
+  )
+endif()
+
+vcpkg_build_make(SUBPATH libcap
+  BUILD_TARGET libcap.pc
+  OPTIONS
+    prefix=${CURRENT_INSTALLED_DIR}
+)


You can use "Target" as a variable to optimize the code.

I am not sure I fully understand, could you elaborate further?

set(TARGETS ***) if(**) list(APPEND TARGETS ***) endif() ... vcpkg_build_make(SUBPATH libcap BUILD_TARGET ${targets} OPTIONS prefix=${CURRENT_INSTALLED_DIR} CC=gcc # use host architecture to generate cap_names.h )

@Adela0814 BUILD_TARGET does not support multi targets in my experience, and this would break cross compilation. So why?

ports/libcap/portfile.cmake

Adela0814 · 2023-05-05T06:11:48Z

ports/libcap/portfile.cmake

+vcpkg_build_make(SUBPATH libcap
+  BUILD_TARGET cap_names.h
+  OPTIONS
+    prefix=${CURRENT_INSTALLED_DIR}
+    CC=gcc # use host architecture to generate cap_names.h
+)
+
+if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
+  vcpkg_build_make(SUBPATH libcap
+    BUILD_TARGET libcap.a
+    OPTIONS
+      prefix=${CURRENT_INSTALLED_DIR}
+  )
+else()
+  vcpkg_build_make(SUBPATH libcap
+    BUILD_TARGET libcap.so
+    OPTIONS
+      prefix=${CURRENT_INSTALLED_DIR}
+  )
+endif()
+
+vcpkg_build_make(SUBPATH libcap
+  BUILD_TARGET libcap.pc
+  OPTIONS
+    prefix=${CURRENT_INSTALLED_DIR}
+)


set(TARGETS ***) if(**) list(APPEND TARGETS ***) endif() ... vcpkg_build_make(SUBPATH libcap BUILD_TARGET ${targets} OPTIONS prefix=${CURRENT_INSTALLED_DIR} CC=gcc # use host architecture to generate cap_names.h )

ports/libsystemd/Config.cmake.in

ports/libcap/configure.patch

Adela0814 · 2023-05-05T08:08:00Z

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags.

jbbjarnason

Address review comments

jbbjarnason · 2023-05-12T08:57:58Z

ports/libcap/portfile.cmake

+vcpkg_build_make(SUBPATH libcap
+  BUILD_TARGET cap_names.h
+  OPTIONS
+    prefix=${CURRENT_INSTALLED_DIR}
+    CC=gcc # use host architecture to generate cap_names.h
+)
+
+if(VCPKG_LIBRARY_LINKAGE STREQUAL "static")
+  vcpkg_build_make(SUBPATH libcap
+    BUILD_TARGET libcap.a
+    OPTIONS
+      prefix=${CURRENT_INSTALLED_DIR}
+  )
+else()
+  vcpkg_build_make(SUBPATH libcap
+    BUILD_TARGET libcap.so
+    OPTIONS
+      prefix=${CURRENT_INSTALLED_DIR}
+  )
+endif()
+
+vcpkg_build_make(SUBPATH libcap
+  BUILD_TARGET libcap.pc
+  OPTIONS
+    prefix=${CURRENT_INSTALLED_DIR}
+)


@Adela0814 BUILD_TARGET does not support multi targets in my experience, and this would break cross compilation. So why?

ports/libsystemd/Config.cmake.in

ports/libcap/cross-compile.patch

jbbjarnason · 2023-05-12T09:31:20Z

ports/libcap/portfile.cmake

+  FETCH_REF "libcap-${VERSION}"
+  REF 3c7dda330bd9a154bb5b878d31fd591e4951fe17
+  PATCHES
+    configure.patch


I have changed it from being a patch to an empty configure script. However I have not made it as a general configure script since it is empty and therefore would instead like to do the required things in CMake.

ports/libcap/configure.patch

dg0yt

The host stuff must be done in the host triplet.
(Example: infoware pci_data.hpp in #31388)

Multiple calls to vcpkg_*_make overwrite logfiles.

Are the pc files valid now?

dg0yt · 2023-05-15T03:21:19Z

ports/libcap/portfile.cmake

+  BUILD_TARGET cap_names.h
+  OPTIONS
+    prefix=${CURRENT_INSTALLED_DIR}
+    CC=gcc # use host architecture to generate cap_names.h


No, this cannot be approved!

Can you elaborate further on this topic please?

@dg0yt Could you please explain?

@Adela0814 or @dg0yt is there any progress with this?

I don't know where to start with explanation again, and why the reviewers can't do it.

Host stuff must be done in the host triplet. The artifact become readily available for the cross build context by using host dependencies. This done in infoware, gmp, nettle, luajit, ..., What shouldn't be assumed to be readily available, even for a linux-only port, is gcc.

The problem with libcap is that it needs a number of makefile variables properly passed in. It is not an easy port to start with.

@dg0yt Okay, I am still unsure what a host triplet is!. Reason for hard coding gcc as compiler is because it is the default compiler for Linux, as you are already aware of I reckon.

As I would like this to finish, I see 3 options for next steps:

Iterative development, this currently works, let's fix it when it becomes a problem.

@Adela0814 helps with the review and me to make host triplet changes.

@dg0yt would you be willing to help by contributing directly to this pull request? I can give you access privileges to my fork.

Which option do you guys prefer?

I have tested the usage of libcap on my Linux machine and it works well. I think this PR can be approved. If there are any bugs in the future, we can create a new PR to fix them.

1. Iterative development, this currently works, let's fix it when it becomes a problem.

Not acceptable IMO. It is known to not be implemented correctly. It may become a problem for another contributor who wants to update lzma, lz4, meson...

2. @Adela0814 helps with the review and me to make host triplet changes.

This is how I understand the roles.

3. @dg0yt would you be willing to help by contributing directly to this pull request? I can give you access privileges to my fork.

I already considered that option. I would be willing but I don't think I can spend time on it soon.

I will be converting your PR to draft status. When this PR is ready for review, please reactivate it.

@Adela0814 Added gcc/clang host compiler detection and logs for each build command are exported correctly.

jbbjarnason · 2023-05-15T08:27:18Z

@dg0yt Thank you for the review!

The host stuff must be done in the host triplet.
(Example: infoware pci_data.hpp in #31388)

I am guessing you are referring to libcap to generate cap_names.h, right? And if so the reason for overriding the compiler is because the libcap Makefiles are not made for cross compiling and utilize the host to run generated artifact to make the cap_names.h. The generated artifact is not used to build the libcap library only the header, in my understanding. This is one way to bypass this problem, I am not sure, is it possible to declare host triplet in ports directory?

Multiple calls to vcpkg_*_make overwrite logfiles.

Should I specify LOGFILE_ROOT to each call?

Are the pc files valid now?

If you are referring to the systemd pkgconfig files, than no. The pkgconfig files are straight from systemd as I don't wan't to change the implementation of systemd without going to upstream with it. As a result I made the cmake configuration file which is an addon, therefore, not changing the behavior in any way.

dan-shaw · 2023-06-15T20:42:50Z

ports/libcap/portfile.cmake

+elseif (CLANG_FOUND)
+  set(C_HOST_COMPILER clang)
+else ()
+  message(FATAL_ERROR "Unable to find gcc or clang host compiler")


As @dg0yt pointed out earlier, this needs to be in the host triplet. If you are cross compiling, you can use VCPKG_CROSSCOMPILING, or compare the host and target triplets. See the protobuf port for an example of this.

Thank you @dan-shaw for the example, and suggestion. I was now able to understand what needed to be changed. I have applied your suggestion.

ports/libcap/vcpkg.json

of libsystemd

* Decouple cap_names.h from libcap.* make targets * Use vcpkg_cmake_get_vars instead of ENV{CC} etc. * Remove unnecessary self dependency of libcap * Update libcap to 1.2.69

jbbjarnason · 2023-09-12T11:18:46Z

@dg0yt That is also possible. @jbbjarnason still working on this?

@dan-shaw I have finished point 2 and 3 of your request for changes comment, cd334f8

dan-shaw · 2023-09-13T22:48:23Z

@dg0yt I think all the outstanding changes have been addressed unless there was something I missed. We may need changes to other ports, but I think it is outside the scope of this PR.

* add libsystemd, libcap, libxcrypt * baseline libsystemd, libcap, libxcrypt * versions tree libsystemd, libcap, libxcrypt * licenses for libsystemd, libcap, libxcrypt in vcpkg.json * update tree for libsystemd, libcap, libxcrypt * allow restricted header libxcrypt * tree allow restricted header libxcrypt * remove message Warning * use targets exist during configure, instead of configure_file variable to search for zstd * update git-tree * add quotes to full paths and remove messages in portfiles * update git-tree * remove extra spaces * update git-tree * remove extra line * update git tree * do configure hack differently and remove cross compile patch and override compiler during build_make * update git tree * try either gcc or clang for libcap and specify log file root for each build command * update git-tree * libcap use host dependency of self * libcap update git tree * libsystemd is LGPL2.1 * update libsystemd git-tree * change from find_package to find_dependency in unofficial cmake export of libsystemd * update libsystemd git-tree * update libxcrypt * update git tree libxcrypt * patch pkgconfig file of libsystemd * update libsystemd git-tree * update libsystemd to 254 * update git-tree libsystemd * * Copy of cap_names.h checked in for cross compilation * Decouple cap_names.h from libcap.* make targets * Use vcpkg_cmake_get_vars instead of ENV{CC} etc. * Remove unnecessary self dependency of libcap * Update libcap to 1.2.69 * Update git-tree * cleanup old versions from version jsons

dg0yt suggested changes Apr 27, 2023

View reviewed changes

LilyWangLL assigned Adela0814 Apr 28, 2023

Adela0814 added category:new-port The issue is requesting a new library to be added; consider making a PR! requires:author-response labels Apr 28, 2023

Adela0814 suggested changes May 4, 2023

View reviewed changes

jbbjarnason requested a review from Adela0814 May 4, 2023 13:08

Adela0814 suggested changes May 5, 2023

View reviewed changes

Adela0814 removed the requires:author-response label May 5, 2023

Adela0814 marked this pull request as draft May 5, 2023 08:08

jbbjarnason commented May 12, 2023

View reviewed changes

jbbjarnason marked this pull request as ready for review May 12, 2023 09:33

jbbjarnason requested review from Adela0814 and dg0yt May 12, 2023 09:34

Adela0814 previously approved these changes May 15, 2023

View reviewed changes

Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label May 15, 2023

dg0yt suggested changes May 15, 2023

View reviewed changes

Adela0814 removed the info:reviewed Pull Request changes follow basic guidelines label May 15, 2023

jbbjarnason requested a review from dg0yt May 16, 2023 09:00

Adela0814 added info:reviewed Pull Request changes follow basic guidelines and removed info:reviewed Pull Request changes follow basic guidelines labels May 24, 2023

Adela0814 marked this pull request as draft May 30, 2023 03:08

jbbjarnason dismissed Adela0814’s stale review via e33e854 May 30, 2023 09:37

jbbjarnason marked this pull request as ready for review May 30, 2023 09:41

jbbjarnason requested a review from Adela0814 May 30, 2023 09:41

Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label May 31, 2023

dan-shaw added the requires:discussion label May 31, 2023

dan-shaw reviewed Jun 15, 2023

View reviewed changes

dan-shaw removed the requires:discussion label Jun 15, 2023

jbbjarnason added 12 commits September 12, 2023 09:15

libcap use host dependency of self

d1aa087

libcap update git tree

7aa28ca

libsystemd is LGPL2.1

5079b13

update libsystemd git-tree

8767244

change from find_package to find_dependency in unofficial cmake export

fe4762d

of libsystemd

update libsystemd git-tree

defb5fe

update libxcrypt

a1d51db

update git tree libxcrypt

2601501

patch pkgconfig file of libsystemd

cfb1fce

update libsystemd git-tree

3c753ed

update libsystemd to 254

6fe4941

update git-tree libsystemd

8715ed4

jbbjarnason force-pushed the add-libsystemd branch from 58f9a8d to 8715ed4 Compare September 12, 2023 09:17

jbbjarnason added 2 commits September 12, 2023 11:12

* Copy of cap_names.h checked in for cross compilation

cd334f8

* Decouple cap_names.h from libcap.* make targets * Use vcpkg_cmake_get_vars instead of ENV{CC} etc. * Remove unnecessary self dependency of libcap * Update libcap to 1.2.69

Update git-tree

608eb19

jbbjarnason marked this pull request as ready for review September 12, 2023 11:19

jbbjarnason requested a review from dan-shaw September 12, 2023 11:19

cleanup old versions from version jsons

e2d19e0

dan-shaw approved these changes Sep 13, 2023

View reviewed changes

Adela0814 approved these changes Sep 14, 2023

View reviewed changes

Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label Sep 14, 2023

dg0yt mentioned this pull request Sep 20, 2023

[sdbus-cpp] New port #33879

Merged

11 tasks

vicroms merged commit 5be487d into microsoft:master Sep 20, 2023
15 checks passed

dg0yt mentioned this pull request Sep 22, 2023

[libcap] Normalize #33923

Merged

dg0yt mentioned this pull request Nov 15, 2023

[dbus] add lz4 as a dependency #35129

Closed

7 tasks

JonLiu1993 mentioned this pull request Nov 17, 2023

[New Port Request] libcap #33216

Closed

dg0yt mentioned this pull request Nov 24, 2023

[vcpkg baseline][czmq] Disable build with systemd #35297

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[libsystemd] add new port #31150

[libsystemd] add new port #31150

jbbjarnason commented Apr 27, 2023

dg0yt left a comment

dg0yt Apr 27, 2023

jbbjarnason Apr 27, 2023

jbbjarnason May 2, 2023 •

edited

Loading

jbbjarnason May 12, 2023

dg0yt Apr 27, 2023

jbbjarnason Apr 27, 2023

Adela0814 May 4, 2023

jbbjarnason May 4, 2023

Adela0814 May 5, 2023

jbbjarnason May 12, 2023

Adela0814 May 5, 2023

Adela0814 commented May 5, 2023

jbbjarnason left a comment

jbbjarnason May 12, 2023

jbbjarnason May 12, 2023

dg0yt left a comment

dg0yt May 15, 2023

jbbjarnason May 15, 2023

Adela0814 May 18, 2023

jbbjarnason May 23, 2023

dg0yt May 23, 2023

jbbjarnason May 24, 2023

Adela0814 May 24, 2023 •

edited

Loading

dg0yt May 25, 2023

Adela0814 May 30, 2023

jbbjarnason May 30, 2023

jbbjarnason commented May 15, 2023

dan-shaw Jun 15, 2023

jbbjarnason Jun 16, 2023

jbbjarnason commented Sep 12, 2023

dan-shaw commented Sep 13, 2023


		vcpkg_fixup_pkgconfig()

		configure_file("${CMAKE_CURRENT_LIST_DIR}/Config.cmake.in" "${CURRENT_PACKAGES_DIR}/share/unofficial-systemd/unofficial-systemd-config.cmake" @ONLY)

[libsystemd] add new port #31150

[libsystemd] add new port #31150

Conversation

jbbjarnason commented Apr 27, 2023

dg0yt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbbjarnason May 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adela0814 commented May 5, 2023

jbbjarnason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dg0yt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Adela0814 May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbbjarnason commented May 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbbjarnason commented Sep 12, 2023

dan-shaw commented Sep 13, 2023

jbbjarnason May 2, 2023 •

edited

Loading

Adela0814 May 24, 2023 •

edited

Loading