Created attachment 162446 [details] [patch] pass --prefix to configure Setting PREFIX doesn't currently pass this information to chrony's configure script. If you run this, for instance, 'make PREFIX=/some/path stage', the stage' target fails since it is looking for files like ${STAGING}/some/path/bin/chronyc and it was installed to ${STAGING}/usr/local/bin/chronyc. Patch to fix this attached.
Mk/bsd.port.mk sets this already for ports that use GNU_CONFIGURE=yes, which it appears net/chrony does not use (instead using HAS_CONFIGURE). @John, can you confirm this passes QA?
Yes, I am familiar with GNU_CONFIGURE. I should have mentioned that I investigated using it instead. However, while the configure script is gnu-like, it's not the same. Using GNU_CONFIGURE sort of works, but by luck more than design. - does not support the target triplet (e.g., "Unrecognized option : i386-portbld-freebsd9.3") - creates at least one incorrect directory when using just GNU_CONFIGURE=yes (with no additional CONFIGURE_ARGS) and PREFIX is specified: [ -d /usr/ports/net/chrony/work/stage/etc ] || mkdir -p /usr/ports/net/chrony/work/stage/etc (instead of work/stage/<PREFIX>/etc) This could be solved by adding --sysconfdir=${PREFIX}/etc to CONFIGURE_ARGS when GNU_CONFIGURE is used. This is currently specified with the current flavor using HAS_CONFIGURE. So, we could switch to GNU_CONFIGURE for this close-but-not-quite-to-gnu-autoconf-flavored configure script with CONFIGURE_ARGS==--sysconfdir=${PREFIX}/etc and live with the target triplet warning message. Or just continue using HAS_CONFIGURE and add --prefix=${PREFIX}. I opted for this choice. I'll attach some QA.
Running with DEVELOPER=yes passes the Q/A checks (tested on 9-stable/i386). ====> Running Q/A tests (stage-qa)
Created attachment 162645 [details] [patch] [v2] add plist mode fix portlint -C exposes an unrelated problem: WARN: /usr/ports/net/chrony/pkg-plist: [9]: unknown pkg-plist directive "@(,,4755) sbin/chronyd"
Comment on attachment 162446 [details] [patch] pass --prefix to configure second patch includes the change from the first patch
Comment on attachment 162645 [details] [patch] [v2] add plist mode fix On second thought, I think that is just a portlint bug. That @(,,4755) seems like perfectly legitimate syntax. However, I'm not sure why this is setuid-root. It seems to me that chronyd should only be started by the root user, not by anyone. In that spirit, I'll submit a patch to just remove the 4755 mode anyway.
Created attachment 162659 [details] [patch][v3] prefix fix + remove setuid-root mode Remove setuid-root mode for chronyd. See comment 6. Given the dire warnings in pkg-message (although I'm not sure it still applies - it's on by default in a number of linux distros now), that seems most prudent anyway. But there's really no reason that I can think of to allow regular users to run chronyd - correct me if that's wrong.
(In reply to John Hein from comment #2) Just to clarify, I wasn't suggesting switching to GNU_CONFIGURE (as i checked the script and confimed it wasnt entirely^W at all gnu-compatible). I just meant that the things that 'are' passed by default 'when' GNU_CONFIGURE is set, could be useful to use in you HAS_CONFIGURE case, in this case --prefix :)
@John and please set maintainer-approval to + when you are done updating it and you want to let people know its 'ready' This probably needs to be MFH'd as well.
Sometimes when I submit a patch, it _seems_ that the maintainer-approval flag is set automatically. So when it doesn't happen, sometimes I forget. I don't understand the cases where it is getting set (or perhaps it's not automatic and I just don't notice someone else setting it). Thanks for the reminder. By the way: testport: OK (9-stable/i386 w/PREFIX=/foo in /usr/local/etc/poudriere.d/make.conf) As far as MFH, I don't think it's urgent to put on the quarterly branch unless the setuid-root removal is considered a security-worthy update. But I don't know where the criteria for the quarterly branch is documented.
@John our auto-assigner should, but doesn't currently, set maintainer-approval to ? for attachments where author=maintainer. It does however set the maintainer-*feedback* flag in this case. I've been setting maintainer-approval ?|+ on attachments manually, which is what I'd like to see maintainers/the system do instead going forward Bug 198271 is the issue that covers updating the auto-assigner to set ? automatically on attachment creation.
Is there any reason not to upgrade to the current version 2.2 ?
Created attachment 163359 [details] patch 1.31.1 -> 2.2 (with the fix from this PR) test-builds fine on 11a, 10.2a+i, 9.3
I have a separate set of changes for updating to 2.2 and was waiting for this change to get committed (it gets rid of post-install in favor using the upstream install-docs, installs the useful info doc, etc.). I'd like to keep that as a separate issue. I'll open a new bug for the 2.2 update if you want to bite that off now, but I'd prefer to commit the focused fix for this bug by itself.
We currently have two patches here, one with a fix, the other with an update. @Kurt, I would highly recommend obsoleting the latter in favour of a separate issue (please feel free to assign to yourself once created) Doing so would leave this issue with the fix only, which can then be merged (MFH) to the latest quarterly, which presumably wants/needs the bugfix also.
Comment on attachment 162659 [details] [patch][v3] prefix fix + remove setuid-root mode Maintainer timeout (22 days), implicit approval
Comment on attachment 163359 [details] patch 1.31.1 -> 2.2 (with the fix from this PR) We'll do this update in a seperate PR
A commit references this bug: Author: pi Date: Mon Nov 23 20:04:21 UTC 2015 New revision: 402324 URL: https://svnweb.freebsd.org/changeset/ports/402324 Log: net/chrony: prefix fix, remove setuid-root mode PR: 204018 Submitted by: John Hein <z7dr6ut7gs@snkmail.com> Reviewed by: koobs Approved by: masaki@club.kyutech.ac.jp (maintainer timeout) Changes: head/net/chrony/Makefile head/net/chrony/pkg-plist
Committed, thanks!
Thanks Kurt, don't forget MFH :)
See bug 204817 for update to 2.2
The MFH to the quartely: obsolete by events.