-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
[libcgroup] added new port #39647
[libcgroup] added new port #39647
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please reduce the indent to 4 spaces.
ports/libcgroup/portfile.cmake
Outdated
--enable-python=no | ||
--enable-tests=no | ||
--enable-samples=no | ||
--enable-systemd=no |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR there is a systemd port in vcpkg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me, that I tried to link systemd from port and encountered some troubles.
But I'll take a look later and give you know about results, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to include systemd support to this port as a feature?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is okay to have it explicitly turned off if it simplifies initial port acceptance.
Much better than auto-detection!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to include systemd support to this port as a feature?
This is an option. But vcpkg CI will normally build only one configuration.
(What is upstream's default? What is the general expectation?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some nuances with systemd. It's an upstream default.
Build with systemd from vcpkg causes some error that I cannot remember now and need to reproduce.
But the build without one breaks an umbrella header. Unfortunately, libcgroup allow to include only this one via include guards in another headers. This error is caused non-installed systemd.h if systemd is disabled.
My attempts to quickly fix this issue in the upstream today were failed because this header (systemd.h) is used from umbrella include (libcgroup.h) by lex/yacc, that has no ifdefs check.
The solutions I see:
- Try again to use systemd from vcpkg and fix encountered errors (if ones will reproduce)
- Explicitly disable systemd and place a patch with removing include guard to allow to include other headers explicitly, but not an umbrella header. As the initial solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dg0yt I found a commit, that fixes a bug I described above. It's not included to v3.1.0 release.
I think it's better to make this port without systemd support and add it later. For now I'll just include this patch in port as initial solution.
Also I reproduced error with building systemd from vcpkg, it's caused by my local setup: CentOS 7 + clang, and it's enough :) On systemd with:
- Python lower than 3.7
- Old automake, autoconf, libtool
- Old stdc++
It's impossible to build libsystemd given in vcpkg's port.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay. It is properly turned off.
ports/libcgroup/vcpkg.json
Outdated
"version-semver": "3.1.0", | ||
"description": "Library for working with cgroup", | ||
"homepage": "https://github.com/libcgroup/libcgroup", | ||
"license": "LGPL-2.1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"license": "LGPL-2.1", | |
"license": "LGPL-2.1-only", |
Do not just apply my suggestion. Also run vcpkg format-manifest ports/libcgroup/vcpkg.json
.
(There is a GH action for this, and it would flag certain issues. But it didn't run yet.)
The header files can be found via |
find_package
calls are REQUIRED, are satisfied byvcpkg.json
's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx.vcpkg.json
matches what upstream says.vcpkg.json
matches what upstream says../vcpkg x-add-version --all
and committing the result.