Bug 204018 - net/chrony: Pass --prefix to ./configure (Make PREFIX safe)
Summary: net/chrony: Pass --prefix to ./configure (Make PREFIX safe)
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Kurt Jaeger
URL:
Keywords: easy, patch, patch-ready
Depends on:
Blocks:
 
Reported: 2015-10-25 18:56 UTC by John Hein
Modified: 2016-01-04 06:01 UTC (History)
3 users (show)

See Also:
bugzilla: maintainer-feedback? (masaki)
koobs: merge-quarterly-


Attachments
[patch] pass --prefix to configure (513 bytes, patch)
2015-10-25 18:56 UTC, John Hein
no flags Details | Diff
[patch] [v2] add plist mode fix (809 bytes, patch)
2015-10-31 19:38 UTC, John Hein
no flags Details | Diff
[patch][v3] prefix fix + remove setuid-root mode (790 bytes, patch)
2015-11-01 00:53 UTC, John Hein
koobs: maintainer-approval+
Details | Diff
patch 1.31.1 -> 2.2 (with the fix from this PR) (8.12 KB, patch)
2015-11-20 20:09 UTC, Kurt Jaeger
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Hein 2015-10-25 18:56:09 UTC
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.
Comment 1 Kubilay Kocak freebsd_committer freebsd_triage 2015-10-26 01:10:57 UTC
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?
Comment 2 John Hein 2015-10-31 19:30:37 UTC
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.
Comment 3 John Hein 2015-10-31 19:31:51 UTC
Running with DEVELOPER=yes passes the Q/A checks (tested on 9-stable/i386).

====> Running Q/A tests (stage-qa)
Comment 4 John Hein 2015-10-31 19:38:31 UTC
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 5 John Hein 2015-10-31 19:40:06 UTC
Comment on attachment 162446 [details]
[patch] pass --prefix to configure

second patch includes the change from the first patch
Comment 6 John Hein 2015-11-01 00:47:27 UTC
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.
Comment 7 John Hein 2015-11-01 00:53:26 UTC
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.
Comment 8 Kubilay Kocak freebsd_committer freebsd_triage 2015-11-01 04:24:32 UTC
(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 :)
Comment 9 Kubilay Kocak freebsd_committer freebsd_triage 2015-11-01 04:26:42 UTC
@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.
Comment 10 John Hein 2015-11-01 22:29:13 UTC
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.
Comment 11 Kubilay Kocak freebsd_committer freebsd_triage 2015-11-02 03:13:15 UTC
@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.
Comment 12 Kurt Jaeger freebsd_committer 2015-11-20 19:57:05 UTC
Is there any reason not to upgrade to the current version 2.2 ?
Comment 13 Kurt Jaeger freebsd_committer 2015-11-20 20:09:19 UTC
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
Comment 14 John Hein 2015-11-23 04:31:36 UTC
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.
Comment 15 Kubilay Kocak freebsd_committer freebsd_triage 2015-11-23 08:19:06 UTC
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 16 Kubilay Kocak freebsd_committer freebsd_triage 2015-11-23 08:19:37 UTC
Comment on attachment 162659 [details]
[patch][v3] prefix fix + remove setuid-root mode

Maintainer timeout (22 days), implicit approval
Comment 17 Kurt Jaeger freebsd_committer 2015-11-23 18:38:38 UTC
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
Comment 18 commit-hook freebsd_committer 2015-11-23 20:05:23 UTC
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
Comment 19 Kurt Jaeger freebsd_committer 2015-11-23 20:06:20 UTC
Committed, thanks!
Comment 20 Kubilay Kocak freebsd_committer freebsd_triage 2015-11-24 02:39:28 UTC
Thanks Kurt, don't forget MFH :)
Comment 21 John Hein 2015-11-25 20:23:14 UTC
See bug 204817 for update to 2.2
Comment 22 Kurt Jaeger freebsd_committer 2016-01-03 21:09:14 UTC
The MFH to the quartely: obsolete by events.