Build: configure: add --with-runstatedir option #136
Conversation
... and use it where appropriate.
| fi | ||
| eval runstatedir="$(eval echo ${runstatedir})" | ||
| AC_SUBST(runstatedir) | ||
|
|
There was a problem hiding this comment.
Did you see the expand_path_option thing Ken has just done in pacemaker?
We should as well consider if we want it synced with .../tests/configure.ac - better of course pull straight so that the copy isn't needed - had some issue i don't recall in detail for which this was a quick hack that persisted.
There was a problem hiding this comment.
Did you see the expand_path_option thing Ken has just done in pacemaker?
Alright, noticed it. We can add the same as well.
We should as well consider if we want it synced with .../tests/configure.ac - better of course pull straight so that the copy isn't needed - had some issue i don't recall in detail for which this was a quick hack that persisted.
OK, will add commits to get this synced with that one when everything is fine.
There was a problem hiding this comment.
yep - will look over it after this PR and try to remember ... in the worst case we can still simply sync ...
| @@ -242,7 +242,7 @@ Defaults to I<enabled>. | |||
| This can be used to override the default watchdog device used and should not | |||
| usually be necessary. | |||
|
|
|||
| =item B<-p> F</var/run/sbd.pid> | |||
| =item B<-p> F</run/sbd.pid> | |||
|
|
|||
There was a problem hiding this comment.
If we put something hacky like
sbd.8.pod: sbd.8.pod.in sbd.sysconfig.pod
sed -e "s,@runstatedir@,$(runstatedir)," $< |sed -e "s/@environment_section@//;t insert;p;d;:insert;rsbd.sysconfig.pod" > $@
in man/Makefile.am we could even make it dynamic ;-)
There was a problem hiding this comment.
Atm I have at least no good idea how to use the autoconf-config-file capabilities here :-(
having environment_section=$(shell cat sbd.sysconfig.pod) probably won't work as we can't get that in as prerequisite.
But maybe with the sed-code that creates sbd.sysconfig.pod ... depends on when/if the vars are evaluated ...
There was a problem hiding this comment.
I haven't figured out a better idea either. It's kind of tricky how any recognized variables are expanded by automake here, but it at least seems to work this way:
sed -e "s,\@runstatedir\@,$(runstatedir)," $< |sed -e "s/@environment_section@//;t insert;p;d;:insert;rsbd.sysconfig.pod" > $@
|
Yep probably we'll need this for now as #135 won't be there quickly enough - and might not replace the pid-file fully - even with systemd (compat with older pacemaker still going for pid-file instead of asking systemd). |
Previously we expanded path variables like:
eval prefix="`eval echo ${prefix}`"
Now, define a helper function expand_path_option for this, to improve
readability and isolate the eval madness. The helper can also take the
option default to reduce code duplication.
Additionally, expand_path_option requires the expanded value to be a full
path (i.e. start with a /), to perform some validation on user-supplied values.
gao-yan
force-pushed
the
configure-with-runstatedir
branch
from
de0ce8e to
515bae0
Compare
| expand_path_option infodir | ||
| expand_path_option mandir | ||
|
|
||
| AS_IF([test x"${runstatedir}" = x""], [runstatedir="${sbd_runstatedir}"]) |
There was a problem hiding this comment.
didn't think of this when reviewing the change in pacemaker but that could actually be further optimized
with expand_path_option having a longer list of parameters at the end where it takes the first !="".
might loose portability though as I didn't find M4sh macros that expand into a loop and shift is a bashism ...
anyway maybe something to note down for next iteration ...
There was a problem hiding this comment.
I guess it'd make expand_path_option() a little complicated/messy ... runstatedir seems kind of treated as a special case here about how the values might come from different sources ...
There was a problem hiding this comment.
As said if M4sh had something like AS_WHILE and some abstraction for shift the change would be insignificant and having various sources with an order doesn't sound if it might not be useful for other things on the longer run.
|
CI is failing because of certificate-issues with some of the builders. |
|
Looks as if overruling --with-runstatedir configuration with what is coming from autoconf via runstatedir might not always be the best idea. |
Alright, I thought the default would be /run from the new autoconf ... What's the general packaging convention in regard of this for new RH/Fedora releases? Is --runstatedir supposed to be always explicitly specified for having the modern way? BTW so far autoconf-2.7x hasn't been in any suse distributions yet.
Option --with-runstatedir was introduced for working with old autoconf, but since so far new autoconf doesn't provide an expected default anyway, probably we could somehow make --runstatedir work with any versions of autoconf? And since pacemaker has the similar, probably this part should better be eventually unified. |
If the runstatedir default is /var/run on any distro with /run, that's clearly a bug in the autoconf packaging. We should report that, even if we come up with a workaround here
Yes, there's a formal packaging guideline that /run should always be used instead of /var/run (whether via --runstatedir or whatever the upstream offers).
autoconf only allows --with-*/--enable-* for custom options. It might be possible to inspect some of the low-level details but I doubt it's worth the trouble. Another wrinkle: Debian backported --runstatedir to autoconf 2.69 We could track whether --with-runstatedir was explicitly set or left to default, and override any built-in runstatedir if explicit |
That is at least what logic would tell. But I didn't research if there is a different convention somehow.
Seems to be the case with c9s as well as autoconf is reporting 2.69 there as well.
Yes, was thinking about that as kind of only way how to handle all versions of autoconf as well. |
|
If we assume a packaging bug with the behavior of runstatedir current logic we have in sbd |
... and use it where appropriate.
Similar to ClusterLabs/pacemaker@36445bcbf1