-
Notifications
You must be signed in to change notification settings - Fork 2k
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
net-misc/kea: various improvements #22116
Conversation
Pull Request assignmentSubmitter: @expeditioneer net-misc/kea: At least one of the listed packages is maintained entirely by non-GitHub developers! Linked bugsNo bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment. If you do not receive any reply to this pull request, please open or link a bug to attract the attention of maintainers. In order to force reassignment and/or bug reference scan, please append Docs: Code of Conduct ● Copyright policy (expl.) ● Devmanual ● GitHub PRs ● Proxy-maint guide |
Pull request CI reportReport generated at: 2021-08-26 17:34 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
+ AC_MSG_ERROR([Cannot find gtest in: $GTEST_PATHS]) | ||
+ fi | ||
+ fi | ||
+ done |
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.
...just use pkg-config?
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.
Done
net-misc/kea/kea-1.8.2-r1.ebuild
Outdated
newinitd "${FILESDIR}"/${PN}-initd-r1 ${PN} | ||
|
||
if use samples; then | ||
diropts -m 0750 -o root -g dhcp |
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.
Indentation is inconsistent
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.
Fixed
systemd_dounit "${FILESDIR}"/kea-dhcp4-server.service | ||
systemd_dounit "${FILESDIR}"/kea-dhcp6-server.service | ||
|
||
newtmpfiles "${FILESDIR}"/${PN}.tmpfiles.conf ${PN}.conf |
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.
Need a pkg_postinst call to tmpfiles_process (should be a QA warning now, see here)
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.
Added missing pkg_postinst
net-misc/kea/kea-1.9.10-r1.ebuild
Outdated
-e 's#test -f "$dir/lib/libgtest.a"#test -f "$dir/lib64/libgtest.a"#g' \ | ||
-e 's#test -f "$dir/lib/libgtest.so"#test -f "$dir/lib64/libgtest.so"#g' \ | ||
-e 's GTEST_LDFLAGS="-L$dir/lib GTEST_LDFLAGS="-L$dir/lib64 g' \ | ||
m4macros/ax_gtest.m4 || die "fixing gtest detection macro failed" |
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.
pkg_config? hardcoding lib64 isn't right here anyway for all platforms.
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.
Done
@@ -9,8 +9,18 @@ | |||
<email>chainsaw@gentoo.org</email> | |||
<name>Tony Vroon</name> | |||
</maintainer> | |||
<maintainer type="person"> |
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.
Make sure you wait until one of the existing maintainers reviews.
Pull request CI reportReport generated at: 2021-08-26 19:54 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
Can you please squash your commits into one? |
@Polynomial-C: I've added the gtest detection via pkg-config and squashed the commits |
Pull request CI reportReport generated at: 2021-08-29 16:39 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
net-misc/kea/kea-1.8.2-r1.ebuild
Outdated
dodir /etc/kea | ||
cp "${FILESDIR}"/kea-ctrl-agent.conf "${ED}"/etc/kea/kea-ctrl-agent.conf || die "Could not create kea-ctrl-agent.conf" | ||
cp "${FILESDIR}"/kea-ddns-server.conf "${ED}"/etc/kea/kea-ddns-server.conf || die "Could not create kea-ddns-server.conf" | ||
cp "${FILESDIR}"/kea-dhcp4.conf "${ED}"/etc/kea/kea-dhcp4.conf || die "Could not create kea kea-dhcp4.conf" | ||
cp "${FILESDIR}"/kea-dhcp6.conf "${ED}"/etc/kea/kea-dhcp6.conf || die "Could not create kea-dhcp6.conf" |
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 replace these with insinto
and doins
calls. That way you may even remove the dodir
line.
Oh and please fix these issues as well:
|
@Polynomial-C this should be fixed now |
Pull request CI reportReport generated at: 2021-08-31 19:25 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
Added bugs to commit message |
Pull request CI reportReport generated at: 2021-09-01 14:45 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
net-misc/kea/kea-1.8.2-r1.ebuild
Outdated
doins "${FILESDIR}"/${PN}-ctrl-agent.conf || die "Could not create kea-ctrl-agent.conf" | ||
doins "${FILESDIR}"/${PN}-ddns-server.conf || die "Could not create kea-ddns-server.conf" | ||
doins "${FILESDIR}"/${PN}-dhcp4.conf || die "Could not create kea kea-dhcp4.conf" | ||
doins "${FILESDIR}"/${PN}-dhcp6.conf || die "Could not create kea-dhcp6.conf" |
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.
doins
does not require || die
calls. See https://devmanual.gentoo.org/ebuild-writing/error-handling/index.html
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.
Done
newinitd "${FILESDIR}"/${PN}-initd-r1 ${PN} | ||
|
||
if use samples; then | ||
diropts -m 0750 -o root -g dhcp |
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.
Alright, removing the dodir
call also made this line useless. But I suppose you want the etc/kea
dir to have these permissions so I suggest to re-introduce the dodir /etc/kea
call.
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.
Done
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.
I've fixed it
3f1278a
to
64dc842
Compare
Pull request CI reportReport generated at: 2021-09-02 20:31 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
@Polynomial-C could we merge this? |
- fi | ||
+ DISTCHECK_GTEST_CONFIGURE_FLAG="--with-gtest" | ||
+ PKG_CHECK_MODULES([GTEST], [gtest], [], [AC_MSG_ERROR([gtest requested but not found])]) | ||
+ GTEST_INCLUDES=`pkg-config --keep-system-cflags --cflags-only-I gtest` |
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.
You shouldn't need to then call pkg-config directly (PKG_CHECK_MODULES should populate everything you need), but if you absolutely must, respect ${PKG_CONFIG} instead (not calling just pkg-config bare).
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.
@thesamesam thank you for the hint with ${PKG_CONFIG} i replaced the plain pkg-config
calls. Sadly PKG_CHECK_MODULES does not provide everything which is required by kea. I also wanted to keep the changes minimal on their codebase.
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.
np, I understand -- thanks! I defer to @Polynomial-C now.
Pull request CI reportReport generated at: 2021-09-06 19:31 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
I've found an issue with the conditional activation of gtest. Please wait with further reviews until i have fixed that. |
The conditional activation of gtest should now work as expected and intended. |
Pull request CI reportReport generated at: 2021-09-07 06:11 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
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 fix the two remaining minor issues.
Optionally you may also bump kea to version 1.9.11
(but in a separate commit please).
Once you've done this, you can commit your PR. Thanks for your patience.
net-misc/kea/kea-1.9.10-r1.ebuild
Outdated
if use samples; then | ||
diropts -m 0750 -o root -g dhcp | ||
dodir /etc/kea | ||
insopts -m 0640 -o root -g dhcp |
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 fix the indentation
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.
done
net-misc/kea/kea-9999.ebuild
Outdated
if use samples; then | ||
diropts -m 0750 -o root -g dhcp | ||
dodir /etc/kea | ||
insopts -m 0640 -o root -g dhcp |
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.
Likewise
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.
done
Pull request CI reportReport generated at: 2021-09-07 18:26 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
fixed-gtest detection added Systemd services Closes: https://bugs.gentoo.org/693332 Closes: https://bugs.gentoo.org/626280 Bug: https://bugs.gentoo.org/751883 Signed-off-by: Dennis Lamm <expeditioneer@gentoo.org> Package-Manager: Portage-3.0.20, Repoman-3.0.3
Signed-off-by: Dennis Lamm <expeditioneer@gentoo.org> Package-Manager: Portage-3.0.20, Repoman-3.0.3
Pull request CI reportReport generated at: 2021-09-07 19:41 UTC There are existing issues already. Please look into the report to make sure none of them affect the packages in question: |
fixed-gtest detection
added Systemd services
Closes: https://bugs.gentoo.org/693332
Closes: https://bugs.gentoo.org/626280
Bug: https://bugs.gentoo.org/751883
Signed-off-by: Dennis Lamm expeditioneer@gentoo.org
Package-Manager: Portage-3.0.20, Repoman-3.0.3