Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for autoconf/automake. #575

Closed
wants to merge 2 commits into from
Closed

Conversation

frobnitzem
Copy link
Contributor

Solves three issues I ran into when executing autogen.sh on my system.

First, pkg-config is in a non-standard location, so I had to copy pkg.m4 into m4. Note this file is copyrighted under the GPL-2, which matches the bubblewrap copyright.

Second, automake 1.16.3 emits warnings about future deprecation of its old behavior - putting object files at the top-level. Silencing these warnings requires changing the AM_INIT_AUTOMAKE call from configure.ac.

Third, the manual rule for test-wrap in Makefile-bwrap.in threw a warning because automake renames the target of things in TESTS.

Note: the full error traceback from autoreconf -i is here:

configure.ac:7: installing 'build-aux/compile'
configure.ac:9: installing 'build-aux/install-sh'
configure.ac:9: installing 'build-aux/missing'
configure.ac:141: installing 'build-aux/tap-driver.sh'
Makefile-bwrap.am:1: warning: source file '$(bwrap_srcpath)/bubblewrap.c' is in a subdirectory,
Makefile-bwrap.am:1: but option 'subdir-objects' is disabled
Makefile.am:32:   'Makefile-bwrap.am' included from here
automake: warning: possible forward-incompatibility.
automake: At least a source file is in a subdirectory, but the 'subdir-objects'
automake: automake option hasn't been enabled.  For now, the corresponding output
automake: object file(s) will be placed in the top-level directory.  However,
automake: this behaviour will change in future Automake versions: they will
automake: unconditionally cause object files to be placed in the same subdirectory
automake: of the corresponding sources.
automake: You are advised to start using 'subdir-objects' option throughout your
automake: project, to avoid future incompatibilities.
Makefile-bwrap.am:1: warning: source file '$(bwrap_srcpath)/bind-mount.c' is in a subdirectory,
Makefile-bwrap.am:1: but option 'subdir-objects' is disabled
Makefile.am:32:   'Makefile-bwrap.am' included from here
Makefile-bwrap.am:1: warning: source file '$(bwrap_srcpath)/network.c' is in a subdirectory,
Makefile-bwrap.am:1: but option 'subdir-objects' is disabled
Makefile.am:32:   'Makefile-bwrap.am' included from here
Makefile-bwrap.am:1: warning: source file '$(bwrap_srcpath)/utils.c' is in a subdirectory,
Makefile-bwrap.am:1: but option 'subdir-objects' is disabled
Makefile.am:32:   'Makefile-bwrap.am' included from here
Makefile.am:54: warning: deprecated feature: target 'test-bwrap' overrides 'test-bwrap$(EXEEXT)'
Makefile.am:54: change your target to read 'test-bwrap$(EXEEXT)'
.../share/automake-1.16/am/program.am: target 'test-bwrap$(EXEEXT)' was defined here
Makefile.am:49:   while processing program 'test-bwrap'
Makefile.am:63: warning: source file 'tests/test-utils.c' is in a subdirectory,
Makefile.am:63: but option 'subdir-objects' is disabled
automake: warning: source file 'tests/try-syscall.c' is in a subdirectory,
automake: but option 'subdir-objects' is disabled
Makefile.am:49:   while processing program 'tests/try-syscall'
Makefile.am: installing 'build-aux/depcomp'
parallel-tests: installing 'build-aux/test-driver'
autoreconf: automake failed with exit status: 1

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A Meson build system was added in 0.6.0 and is working pretty well by now, so I'm very tempted to remove the Autotools build system in preference to merging changes into it; so spending time on improving the Autotools build system might not be very productive.

First, pkg-config is in a non-standard location, so I had to copy pkg.m4 into m4.

Please set the environment variable ACLOCAL_PATH to point to where your pkg.m4 is, instead of copying pkg.m4 into every project you compile, which scales poorly - if you only compile 5 projects, then that's 5 maintainers who have to spend time looking at that change, and then review requests to update the bundled copy for the rest of time.

Makefile.am Outdated
@@ -51,7 +51,7 @@ test_extra_programs = \
tests/try-syscall \
$(NULL)

test-bwrap: bwrap
test-bwrap$(EXEEXT): bwrap
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part looks fine, but please make it its own commit. If you find yourself writing out a list in a commit message, that's often a sign that it should have been more than one commit.

@@ -6,7 +6,7 @@ AC_CONFIG_AUX_DIR([build-aux])

AC_USE_SYSTEM_EXTENSIONS

AM_INIT_AUTOMAKE([1.11 -Wno-portability foreign no-define tar-ustar no-dist-gzip dist-xz])
AM_INIT_AUTOMAKE([1.11 -Wno-portability foreign no-define tar-ustar no-dist-gzip dist-xz subdir-objects])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this part looks fine, but should be its own commit.

@eli-schwartz
Copy link

Please set the environment variable ACLOCAL_PATH to point to where your pkg.m4 is, instead of copying pkg.m4 into every project you compile, which scales poorly - if you only compile 5 projects, then that's 5 maintainers who have to spend time looking at that change, and then review requests to update the bundled copy for the rest of time.

Indeed, particularly because this is a maintainer-only task and pkg.m4 is not needed when using the release tarballs e.g. bubblewrap-0.8.0.tar.xz, which include a copy of pkg.m4 (inlined into aclocal.m4) as well as the final generated configure script with pkg-config support.

@frobnitzem
Copy link
Contributor Author

I have removed the pkg.m4 copied-in here. Now only 2 lines are changed.

@frobnitzem frobnitzem requested a review from smcv May 8, 2023 16:16
@smcv
Copy link
Collaborator

smcv commented Feb 15, 2024

Did you see the reviews where I said "this part is fine, but should be its own commit"? Those parts were fine, but should have been their own commits.

@smcv
Copy link
Collaborator

smcv commented Feb 15, 2024

I've split up the commit myself. I hope that illustrates what I meant by "If you find yourself writing out a list in a commit message, that's often a sign that it should have been more than one commit".

@smcv
Copy link
Collaborator

smcv commented Feb 15, 2024

Unfortunately, distcheck is now failing in CI:


make[1]: Entering directory '/home/runner/work/bubblewrap/bubblewrap/_build/bubblewrap-0.8.0/_build/sub'
/usr/bin/mkdir: cannot create directory ‘../../.deps’: Permission denied
/usr/bin/mkdir: cannot create directory ‘../../.deps’: Permission denied
make[1]: *** [Makefile:777: ../../.deps/bwrap-network.Po] Error 1
make[1]: *** Waiting for unfinished jobs....
make[1]: *** [Makefile:777: ../../.deps/bwrap-utils.Po] Error 1
/usr/bin/mkdir: cannot create directory ‘../../.deps’: Permission denied
make[1]: *** [Makefile:777: ../../.deps/bwrap-bubblewrap.Po] Error 1
/usr/bin/mkdir: cannot create directory ‘../../.deps’: Permission denied
make[1]: *** [Makefile:777: ../../.deps/bwrap-bind-mount.Po] Error 1
make[1]: Leaving directory '/home/runner/work/bubblewrap/bubblewrap/_build/bubblewrap-0.8.0/_build/sub'
make: *** [Makefile:1281: distcheck] Error 1
make: Leaving directory '/home/runner/work/bubblewrap/bubblewrap/_build'

I'm very tempted to resolve this by deleting the Autotools build system, leaving only Meson, rather than by reviewing or testing further Autotools changes.

@smcv
Copy link
Collaborator

smcv commented Feb 15, 2024

The part that I've split into commit "Silence Automake warning for items in TESTS being renamed" seems fine. I've opened #622 to land that.

The part that adds subdir-objects seems to be breaking make distcheck. If you still want this change, please investigate.

Copy link
Collaborator

@smcv smcv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said previously, the remaining change here is failing distcheck, which needs addressing before it can be merged.

automake 1.16.3 emits warnings about future deprecation of its old
behavior - putting object files at the top-level. This project does not
rely on the old behavior, so we can silence the warning by opting in to
the new behavior.

Signed-off-by: David M. Rogers <[email protected]>
[smcv: Split out from a larger commit, re-worded commit message]
Signed-off-by: Simon McVittie <[email protected]>
@frobnitzem
Copy link
Contributor Author

The autotools build log indicates a permissions error

162
make[1]: Entering directory '/home/runner/work/bubblewrap/bubblewrap/_build/bubblewrap-0.8.0/_build/sub'
/usr/bin/mkdir: cannot create directory ‘../../.deps’: Permission denied

It is related to the chmod/mkdir sequence earlier:

40
chmod -R a-w bubblewrap-0.8.0
chmod u+w bubblewrap-0.8.0
mkdir bubblewrap-0.8.0/_build bubblewrap-0.8.0/_build/sub bubblewrap-0.8.0/_inst
chmod a-w bubblewrap-0.8.0

I'll look for a fix that creates a .deps directory at that step.

@frobnitzem frobnitzem force-pushed the autoconf branch 2 times, most recently from fc6b4e9 to 6305fcb Compare March 15, 2024 21:40
@smcv
Copy link
Collaborator

smcv commented Mar 16, 2024

It is intentional that distcheck runs with the source directory non-writeable: it's trying to make sure that an out-of-tree (srcdir != builddir) build doesn't alter the source code, only the build directory. This is a general Autotools thing, not specific to bubblewrap.

@eli-schwartz
Copy link

A Meson build system was added in 0.6.0 and is working pretty well by now, so I'm very tempted to remove the Autotools build system in preference to merging changes into it; so spending time on improving the Autotools build system might not be very productive.

People have now had two years and multiple releases to migrate to meson.

It might be worth asking what the reason for continuing to use autotools here is, before proceeding further. I would have assumed that if there were lingering issues with the meson conversion they would have been discovered by now, so "teething troubles" should not be the answer...

@mcatanzaro
Copy link

Probably best to close this and land #625 instead.

@eli-schwartz
Copy link

Yeah, I'm still wondering what the original reason was for trying to improve the autotools build which is already deprecated regardless.

It would be good to know if people are actually running into issues with the port to meson, but since no one has actually reported such an issue, it feels like we should assume there are no issues and declare success, then delete autotools.

@smcv
Copy link
Collaborator

smcv commented Oct 1, 2024

It would be good to know if people are actually running into issues with the port to meson, but since no one has actually reported such an issue, it feels like we should assume there are no issues and declare success, then delete autotools.

Done after 0.10.0, so this PR is no longer applicable.

@smcv smcv closed this Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants