diff mbox series

[Openvpn-devel,1/3] Conditionally add subdir-objects option to automake

Message ID 20230204004512.250271-1-selva.nair@gmail.com
State Accepted
Headers show
Series [Openvpn-devel,1/3] Conditionally add subdir-objects option to automake | expand

Commit Message

Selva Nair Feb. 4, 2023, 12:45 a.m. UTC 
From: Selva Nair <selva.nair@gmail.com>

- Eliminates repeated warnings such as
  warning: source file '$(openvpn_srcdir)/env_set.c' is in a subdirectory,
  but option 'subdir-objects' is disabled
- Enabled only for automake >= 1.16 as older versions have a buggy implementation
  of this option

Main side effect of this option is that object files like openvpnserv-blockdns.o
are now created in src/openvpn where block-dns.c resides instead of in
src/openvpnserv. Same for object files for sources from $(openvpn_srcdir) compiled
into test executables.

See also past discussion on this topic:
  https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg00013.html

Signed-off-by: Selva Nair <selva.nair@gmail.com>
---
 configure.ac | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

Comments

Arne Schwabe Feb. 5, 2023, 9:46 a.m. UTC | #1 
Am 04.02.23 um 01:45 schrieb selva.nair@gmail.com:
> From: Selva Nair <selva.nair@gmail.com>
> 
> - Eliminates repeated warnings such as
>    warning: source file '$(openvpn_srcdir)/env_set.c' is in a subdirectory,
>    but option 'subdir-objects' is disabled
> - Enabled only for automake >= 1.16 as older versions have a buggy implementation
>    of this option
> 
> Main side effect of this option is that object files like openvpnserv-blockdns.o
> are now created in src/openvpn where block-dns.c resides instead of in
> src/openvpnserv. Same for object files for sources from $(openvpn_srcdir) compiled
> into test executables.
> 
> See also past discussion on this topic:
>    https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg00013.html
> 

Acked-By: Arne Schwabe <arne@rfc2549.org>
Gert Doering Feb. 10, 2023, 6:59 p.m. UTC | #2 
This warning has been quite an annoyance for some time - thanks for
taking care of it, and also figuring out what caveats apply.  I do
not understand autoconf intricacies, but I have fed this to github and
to the buildbots, and they all seem to agree "things still build fine" :-)
(the oldest buildbot is FreeBSD 7.4 with automake-1.11, and then there's
the OpenIndiana buildbot with automake-1.15.1, so we have everything
in the mix... the "current" OSes in the zoo all have 1.16.2 or later)

Your patch has been applied to the master and release/2.6 branch.

commit 8b915c48252da81b96041de66847272b0902c755 (master)
commit 7f72abcf8a56bb35a510a3409e03a4e2aaba50da (release/2.6)
Author: Selva Nair
Date:   Fri Feb 3 19:45:10 2023 -0500

     Conditionally add subdir-objects option to automake

     Signed-off-by: Selva Nair <selva.nair@gmail.com>
     Acked-by: Arne Schwabe <arne@rfc2549.org>
     Message-Id: <20230204004512.250271-1-selva.nair@gmail.com>
     URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg26147.html
     Signed-off-by: Gert Doering <gert@greenie.muc.de>


--
kind regards,

Gert Doering
Gert Doering March 8, 2023, 2:40 p.m. UTC | #3 
Hi,

On Fri, Feb 10, 2023 at 07:59:03PM +0100, Gert Doering wrote:
> This warning has been quite an annoyance for some time - thanks for
> taking care of it, and also figuring out what caveats apply.  I do
> not understand autoconf intricacies, but I have fed this to github and
> to the buildbots, and they all seem to agree "things still build fine" :-)
> (the oldest buildbot is FreeBSD 7.4 with automake-1.11, and then there's
> the OpenIndiana buildbot with automake-1.15.1, so we have everything
> in the mix... the "current" OSes in the zoo all have 1.16.2 or later)
> 
> Your patch has been applied to the master and release/2.6 branch.
> 
> commit 8b915c48252da81b96041de66847272b0902c755 (master)
> commit 7f72abcf8a56bb35a510a3409e03a4e2aaba50da (release/2.6)
> Author: Selva Nair
> Date:   Fri Feb 3 19:45:10 2023 -0500
> 
>      Conditionally add subdir-objects option to automake


This patch brought with it a somewhat unexpected and unexplainable (to me
and Frank, that is) side effect - "make distcheck" explodes:

gert@gentoo ~/openvpn-26.git $ git pull --rebase
Already up to date.
gert@gentoo ~/openvpn-26.git $ make distcheck
...
config.status: creating config.h
config.status: creating include/openvpn-plugin.h
config.status: executing depfiles commands
config.status: error: in `/home/gert/openvpn-26.git/openvpn-2.6.1/_build/sub':
config.status: error: Something went wrong bootstrapping makefile fragments
    for automatic dependency tracking.  If GNU make was not used, consider
    re-running the configure script with MAKE="gmake" (or whatever is
    necessary).  You can also try re-running configure with the
    '--disable-dependency-tracking' option to at least be able to build
    the package (albeit without support for automatic dependency tracking).
See `config.log' for more details
make: *** [Makefile:834: distcheck] Error 1

the relevant "config.log" is hiding, but there are some errors that look
relevant:

gert@gentoo ~/openvpn-26.git/openvpn-2.6.1/_build/sub $ less config.log
...
config.status:1694: cd tests/unit_tests/plugins/auth-pam       && sed -e '/# am-
-include-marker/d' Makefile         | make -f - am--depfiles
make[1]: Entering directory '/home/gert/openvpn-26.git/openvpn-2.6.1/_build/sub/
tests/unit_tests/plugins/auth-pam'
/bin/mkdir: cannot create directory '../../../../../../src/plugins/auth-pam/.dep
s': Permission denied

(many of them)


Not sure what to do about it... for now we've decided to move onwards
with the 2.6.1 release, as the "make dist" built tarball passes all
the tests - including "out of tree" builds - so it's not crucially
urgent ("yesterday" urgent), but it would be good to get this fixed again.

Anyone who understands autoconf...?

(This is on Gentoo, with automake 1.16.5, autoconf 2.71, libtool 2.4.7)

gert
Frank Lichtenheld March 8, 2023, 2:58 p.m. UTC | #4 
On Wed, Mar 08, 2023 at 03:40:42PM +0100, Gert Doering wrote:
> Not sure what to do about it... for now we've decided to move onwards
> with the 2.6.1 release, as the "make dist" built tarball passes all
> the tests - including "out of tree" builds - so it's not crucially
> urgent ("yesterday" urgent), but it would be good to get this fixed again.
> 
> Anyone who understands autoconf...?

So I have a fix %)

Turns out one needs to read the automake release notes *very* carefully. When
they fixed the issues with subdir-objects in 1.16.1 they wrote:

The 'subdir-object' option no longer causes object files corresponding
to source files specified with an explicit '$(srcdir)' component to be
placed in the source tree rather than in the build tree.

And they wrote

Automatic dependency tracking has been fixed to work also when the
'subdir-object' option is used and some 'foo_SOURCES' definition
contains unexpanded references to make variables, as in, e.g.:

  a_src = sources/libs/aaa
  b_src = sources/bbb
  foo_SOURCES = $(a_src)/bar.c $(b_src)/baz.c

With such a setup, the created makefile fragment containing dependency
tracking information will be correctly placed under the directories
named 'sources/libs/aaa/.deps' and 'sources/bbb/.deps', rather than
mistakenly under directories named (literally!) '$(src_a)/.deps' and
'$(src_b)/.deps'

What they did not explicitely mention is that the fixes do not work
*together*. So if you have $(a_src) in foo_SOURCES the .deps directory
will be created in the *source* tree, not the *build* tree. Only if you
have literally srcdir or top_srcdir in SOURCES, then it puts them in
the build tree.

That breaks make distcheck.

So we need to apply fixes like this:
-auth_pam_testdriver_SOURCES = test_search_and_replace.c  $(sut_sourcedir)/utils.h $(sut_sourcedir)/utils.c
+auth_pam_testdriver_SOURCES = test_search_and_replace.c  $(top_srcdir)/src/plugins/auth-pam/utils.h $(top_srcdir)/src/plugins/auth-pam/utils.c

Will send a patch.

Regards,
diff mbox series

Patch

diff --git a/configure.ac b/configure.ac
index 91500087..95d795c3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -54,9 +54,22 @@  m4_define([serial_tests], [
                 awk '{split ($NF,a,"."); if (a[1] == 1 && a[2] >= 12) { print "serial-tests" }}'
     ])
 ])
+
+dnl Automake 1.14+ warns if sources are in sub-directories but subdir-objects
+dnl options is not enabled. However, automake before 1.15a has a bug that causes
+dnl variable expansion to fail in foo_SOURCES when this option is used.
+dnl As most of our build systems are now likely to use automake 1.16+ add a
+dnl work around to conditionally add subdir-objects option.
+m4_define([subdir_objects], [
+    m4_esyscmd([automake --version |
+                head -1 |
+                awk '{split ($NF,a,"."); if (a[1] == 1 && a[2] >= 16) { print "subdir-objects" }}'
+    ])
+])
+
 # This foreign option prevents autoreconf from overriding our COPYING and
 # INSTALL targets:
-AM_INIT_AUTOMAKE(foreign serial_tests 1.9) dnl NB: Do not [quote] this parameter.
+AM_INIT_AUTOMAKE(foreign serial_tests subdir_objects 1.9) dnl NB: Do not [quote] this parameter.
 AC_CANONICAL_HOST
 AC_USE_SYSTEM_EXTENSIONS