diff mbox series

[bug#71022] configure.ac: Set default value for the 'prefix' variable.

Message ID 9ac2df37867548ae5e15e4c262458d8fc63ba9a1.1715989953.git.maxim.cournoyer@gmail.com
State New
Headers show
Series [bug#71022] configure.ac: Set default value for the 'prefix' variable. | expand

Commit Message

Maxim Cournoyer May 17, 2024, 11:52 p.m. UTC
The Guix standard configuration uses a localstatedir of /var and a sysconfdir
of /etc.  To ease things for everyone, make the default values match that
standard expected configuration.  See
<https://lists.gnu.org/archive/html/guix-devel/2024-05/msg00003.html> for a
related discussion.

* configure.ac: Default $prefix to '' unless already set.
* doc/contributing.texi (Building from Git): Streamline doc.
* doc/guix-cookbook.texi (Guix environment via direnv): Likewise.

Change-Id: I23cd12b58a842d246fbc9fdc740311c573eb0212
---

 configure.ac           |  4 ++++
 doc/contributing.texi  | 15 +--------------
 doc/guix-cookbook.texi |  2 +-
 3 files changed, 6 insertions(+), 15 deletions(-)


base-commit: 5a624adfd7b14c3717237d137bd0766c77f0f570

Comments

Ricardo Wurmus May 19, 2024, 10:42 a.m. UTC | #1
Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> -When configuring Guix on a system that already has a Guix installation,
> -be sure to specify the same state directory as the existing installation
> -using the @option{--localstatedir} option of the @command{configure}
> -script (@pxref{Directory Variables, @code{localstatedir},, standards,
> -GNU Coding Standards}).  Usually, this @var{localstatedir} option is set
> -to the value @file{/var}.  The @command{configure} script protects
> -against unintended misconfiguration of @var{localstatedir} so you do not
> -inadvertently corrupt your store (@pxref{The Store}).  The configuration
> -directory should also be configured by setting the @option{--sysconfdir}
> -option to the @file{/etc} value, which is the location used by Guix to
> -store for example the access control list of authorized machines and the
> -definition of offload machines.

This information is still useful, isn't it?  It's important for people
who intend to modify these locations, such as system administrators who
might want to install things elsewhere.  Perhaps this could be rewritten
to explain why this would be a bad idea.
Ludovic Courtès May 19, 2024, 2:16 p.m. UTC | #2
Hi Maxim,

Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

> The Guix standard configuration uses a localstatedir of /var and a sysconfdir
> of /etc.  To ease things for everyone, make the default values match that
> standard expected configuration.  See
> <https://lists.gnu.org/archive/html/guix-devel/2024-05/msg00003.html> for a
> related discussion.
>
> * configure.ac: Default $prefix to '' unless already set.
> * doc/contributing.texi (Building from Git): Streamline doc.
> * doc/guix-cookbook.texi (Guix environment via direnv): Likewise.
>
> Change-Id: I23cd12b58a842d246fbc9fdc740311c573eb0212

[...]

> +dnl Set some sane default directory variables for use with the Guix.  This
> +dnl also causes localstatedir to be /var and sysconfdir to be /etc.
> +test "$prefix" = NONE && prefix=

Shouldn’t it read “prefix=/”?

Might be good to check whether this breaks “make distcheck”, so I
suppose it should be fine.

Apart from that it LGTM.  Thanks for this patch!

Ludo’.
Maxim Cournoyer May 22, 2024, 12:16 a.m. UTC | #3
Hi Ricardo,

Ricardo Wurmus <rekado@elephly.net> writes:

> Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:
>
>> -When configuring Guix on a system that already has a Guix installation,
>> -be sure to specify the same state directory as the existing installation
>> -using the @option{--localstatedir} option of the @command{configure}
>> -script (@pxref{Directory Variables, @code{localstatedir},, standards,
>> -GNU Coding Standards}).  Usually, this @var{localstatedir} option is set
>> -to the value @file{/var}.  The @command{configure} script protects
>> -against unintended misconfiguration of @var{localstatedir} so you do not
>> -inadvertently corrupt your store (@pxref{The Store}).  The configuration
>> -directory should also be configured by setting the @option{--sysconfdir}
>> -option to the @file{/etc} value, which is the location used by Guix to
>> -store for example the access control list of authorized machines and the
>> -definition of offload machines.
>
> This information is still useful, isn't it?  It's important for people
> who intend to modify these locations, such as system administrators who
> might want to install things elsewhere.  Perhaps this could be rewritten
> to explain why this would be a bad idea.

It's useful, but it's duplicated with the warning you'd get from
the GUIX_CHECK_LOCALSTATEDIR m4/guix.m4 macro:

--8<---------------cut here---------------start------------->8---
dnl GUIX_CHECK_LOCALSTATEDIR
dnl
dnl Check that the LOCALSTATEDIR value is consistent with that of the existing
dnl Guix installation, if any.  Error out or warn if they do not match.
AC_DEFUN([GUIX_CHECK_LOCALSTATEDIR], [
  AC_REQUIRE([GUIX_CURRENT_LOCALSTATEDIR])
  if test "x$guix_cv_current_localstatedir" != "xnone"; then
    if test "$guix_cv_current_localstatedir" != "$guix_localstatedir"; then
      case "$localstatedir" in
        NONE|\${prefix}*)
          # User kept the default value---i.e., did not pass '--localstatedir'.
          AC_MSG_ERROR([chosen localstatedir '$guix_localstatedir' does not match \
that of the existing installation '$guix_cv_current_localstatedir'
Installing may corrupt $storedir!
Use './configure --localstatedir=$guix_cv_current_localstatedir'.])
          ;;
        *)
          # User passed an explicit '--localstatedir'.  Assume they know what
          # they're doing.
          AC_MSG_WARN([chosen localstatedir '$guix_localstatedir' does not match \
that of the existing installation '$guix_cv_current_localstatedir'])
          AC_MSG_WARN([installing may corrupt $storedir!])
         ;;
      esac
    fi
  fi])
--8<---------------cut here---------------end--------------->8---

So it seems judicious to remove it, especially since with this change
the default `./configure` would no longer require the user to think
about '--localstatedir' and friends (unless their system was an odd
snowflake, in which case the macro would do its job to warn them).

Does that clarify the intent?
Maxim Cournoyer May 22, 2024, 12:24 a.m. UTC | #4
Hi,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

> The Guix standard configuration uses a localstatedir of /var and a sysconfdir
> of /etc.  To ease things for everyone, make the default values match that
> standard expected configuration.  See
> <https://lists.gnu.org/archive/html/guix-devel/2024-05/msg00003.html> for a
> related discussion.
>
> * configure.ac: Default $prefix to '' unless already set.
> * doc/contributing.texi (Building from Git): Streamline doc.
> * doc/guix-cookbook.texi (Guix environment via direnv): Likewise.
>
> Change-Id: I23cd12b58a842d246fbc9fdc740311c573eb0212

While authoring the above, I got thinking about *why* we can't default
to the default GNU /usr/local prefix, since Guix *should* handle this
correctly, via paths registered in the (guix config).  I guess the only
reason is that /var is the hard-coded $localstatedir value we chose for
any Guix package and can't meaningfully be changed (without rewritting
the graft), so if it must exists anyway, we might as well use it for
Guix itself by default.

In other words, I believe this change does the right thing (the other
idea I was musing with defaulting '--prefix=' or '--localstatedir' to
the value of any installed guix, unless the user had provided one
explicitly).
Maxim Cournoyer May 22, 2024, 12:30 p.m. UTC | #5
Hi Ludovic,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> The Guix standard configuration uses a localstatedir of /var and a sysconfdir
>> of /etc.  To ease things for everyone, make the default values match that
>> standard expected configuration.  See
>> <https://lists.gnu.org/archive/html/guix-devel/2024-05/msg00003.html> for a
>> related discussion.
>>
>> * configure.ac: Default $prefix to '' unless already set.
>> * doc/contributing.texi (Building from Git): Streamline doc.
>> * doc/guix-cookbook.texi (Guix environment via direnv): Likewise.
>>
>> Change-Id: I23cd12b58a842d246fbc9fdc740311c573eb0212
>
> [...]
>
>> +dnl Set some sane default directory variables for use with the Guix.  This
>> +dnl also causes localstatedir to be /var and sysconfdir to be /etc.
>> +test "$prefix" = NONE && prefix=
>
> Shouldn’t it read “prefix=/”?

This is what I went for originally, but it would cause this error:

--8<---------------cut here---------------start------------->8---
configure: error: chosen localstatedir '//var' does not match that of the existing installation '/var'
Installing may corrupt /gnu/store!
Use './configure --localstatedir=/var'.
--8<---------------cut here---------------end--------------->8---

So it seems Autoconf's generated configure script expects --prefix to
*not* contain a trailing '/', which is the case for --prefix=/.

> Might be good to check whether this breaks “make distcheck”, so I
> suppose it should be fine.

Good suggestion; I just tested the target and the tarball was
successfully generated.

> Apart from that it LGTM.  Thanks for this patch!

Thanks for reviewing it!  I'll send a v2, and wait for further comments,
if any.
Ludovic Courtès May 23, 2024, 9:17 a.m. UTC | #6
Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:

>> Shouldn’t it read “prefix=/”?
>
> This is what I went for originally, but it would cause this error:
>
> configure: error: chosen localstatedir '//var' does not match that of the existing installation '/var'
> Installing may corrupt /gnu/store!
> Use './configure --localstatedir=/var'.
>
> So it seems Autoconf's generated configure script expects --prefix to
> *not* contain a trailing '/', which is the case for --prefix=/.

Oh I see, makes sense.

Thanks,
Ludo’.
diff mbox series

Patch

diff --git a/configure.ac b/configure.ac
index 8c3a06da37..f831416650 100644
--- a/configure.ac
+++ b/configure.ac
@@ -73,6 +73,10 @@  AC_ARG_ENABLE([daemon],
   [guix_build_daemon="$enableval"],
   [guix_build_daemon="yes"])
 
+dnl Set some sane default directory variables for use with the Guix.  This
+dnl also causes localstatedir to be /var and sysconfdir to be /etc.
+test "$prefix" = NONE && prefix=
+
 # Prepare a version of $localstatedir & co. that does not contain references
 # to shell variables.
 guix_prefix="`eval echo $prefix | sed -e"s|NONE|/usr/local|g"`"
diff --git a/doc/contributing.texi b/doc/contributing.texi
index 66f4e86d0a..539b4dd0d3 100644
--- a/doc/contributing.texi
+++ b/doc/contributing.texi
@@ -235,7 +235,7 @@  Building from Git
 Then, run:
 
 @example
-./configure --localstatedir=/var --sysconfdir=/etc
+./configure
 @end example
 
 @noindent
@@ -246,19 +246,6 @@  Building from Git
 important to pass the right @code{localstatedir} and @code{sysconfdir}
 values, which get recorded in the @code{(guix config)} Guile module.
 
-When configuring Guix on a system that already has a Guix installation,
-be sure to specify the same state directory as the existing installation
-using the @option{--localstatedir} option of the @command{configure}
-script (@pxref{Directory Variables, @code{localstatedir},, standards,
-GNU Coding Standards}).  Usually, this @var{localstatedir} option is set
-to the value @file{/var}.  The @command{configure} script protects
-against unintended misconfiguration of @var{localstatedir} so you do not
-inadvertently corrupt your store (@pxref{The Store}).  The configuration
-directory should also be configured by setting the @option{--sysconfdir}
-option to the @file{/etc} value, which is the location used by Guix to
-store for example the access control list of authorized machines and the
-definition of offload machines.
-
 Finally, you can build Guix and, if you feel so inclined, run the tests
 (@pxref{Running the Test Suite}):
 
diff --git a/doc/guix-cookbook.texi b/doc/guix-cookbook.texi
index 3bc63cba7a..cbd088632a 100644
--- a/doc/guix-cookbook.texi
+++ b/doc/guix-cookbook.texi
@@ -5076,7 +5076,7 @@  Guix environment via direnv
     # Predefine configure flags.
     configure()
     @{
-        ./configure --localstatedir=/var --prefix=
+        ./configure
     @}
     export_function configure