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

autoconf 2.71 (+ automake update) #73797

Closed
wants to merge 2 commits into from
Closed

Conversation

carlocab
Copy link
Member

@carlocab carlocab commented Mar 24, 2021

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

autoconf now needs to depend on brewed m4, or else automake's configure script will complain about a broken autoconf.

Alternatively, we could just depend on brewed m4 only for the automake build in order to mislead automake's configure script, but that could lead to other broken things in case automake's test for a working autoconf was not a false positive. (I haven't actually tested this, but I think it should work.)

Follow up to #66511. See also #70617.


TODO

@BrewTestBot BrewTestBot added the automerge-skip `brew pr-automerge` will skip this pull request label Mar 24, 2021
@carlocab
Copy link
Member Author

carlocab commented Mar 24, 2021

It looks like superenv forces autoconf to try to use the macOS built-in m4 here:

    self["M4"] = DevelopmentTools.locate("m4") if deps.any? { |d| d.name == "autoconf" }

We could either:

  1. keep that, and just trick automake's configure script here by using brewed m4 just for the automake build (and autoconf probably no longer needs to depend on m4); or,
  2. change the above line in superenv, in case automake was right to complain about the built-in macOS m4.

autoconf actually builds and tests fine without brewed m4; it only fails when you try to build automake, or run #{pkgshare}/autotest/autotest.m4 (this looks a lot like what automake is doing in its configure script).

Thoughts, @Bo98, @fxcoudert?

@Bo98
Copy link
Member

Bo98 commented Mar 24, 2021

We could either:

  1. keep that, and just trick automake's configure script here by using brewed m4 just for the automake build (and autoconf probably no longer needs to depend on m4); or,
  2. change the above line in superenv, in case automake was right to complain about the built-in macOS m4.

That's an interesting debate actually. I know licenses haven't traditionally been too much of a concern, but using brewed m4 everywhere would mean anyone with HOMEBREW_FORBIDDEN_LICENSES=GPL-3.0 can no longer build formulae using autotools.

@carlocab
Copy link
Member Author

carlocab commented Mar 24, 2021

Well, there's also the question of whether automake's configure script is wrong when it says autoconf + macOS m4 is broken. If it isn't, then I guess we don't really have much of a choice on brewed m4 everywhere.

It'll also lead to more headaches if other build scripts use the same autoconf test as automake's does, but I guess that shouldn't be too common.

@carlocab
Copy link
Member Author

That's an interesting debate actually. I know licenses haven't traditionally been too much of a concern, but using brewed m4 everywhere would mean anyone with HOMEBREW_FORBIDDEN_LICENSES=GPL-3.0 can no longer build formulae using autotools.

Actually, autoconf is GPL-3.0-or-later now, so it seems we don't really have a choice on the licensing issue:

❯ autoconf --version
autoconf (GNU Autoconf) 2.71
Copyright (C) 2021 Free Software Foundation, Inc.
License GPLv3+/Autoconf: GNU GPL version 3 or later
<https://gnu.org/licenses/gpl.html>, <https://gnu.org/licenses/exceptions.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

Written by David J. MacKenzie and Akim Demaille.

@Bo98
Copy link
Member

Bo98 commented Mar 24, 2021

In that case, ignore what I said about that.

@Bo98
Copy link
Member

Bo98 commented Mar 24, 2021

So I looked into this. This stems from a bug in m4 where it loses track of the current file and line under some situations. I'm not entirely sure when it was fixed. 1.4.8 revamped how line tracking worked so it could well be that version fixed it (I still don't know why Apple's version doesn't have these changes since the GPLv3 move happened in 1.4.10).

You can see the bug here:

$ /usr/bin/m4 --nesting-limit=1024 --include=/usr/local/Cellar/autoconf/2.71/share/autoconf --debug=aflqi --fatal-warning --trace=_m4_warn --reload-state=/usr/local/Cellar/autoconf/2.71/share/autoconf/autoconf/autoconf.m4f /usr/local/Cellar/autoconf/2.71/share/autoconf/autoconf/trailer.m4 conftest.ac > /dev/null
m4debug: input read from /usr/local/Cellar/autoconf/2.71/share/autoconf/autoconf/trailer.m4
m4debug:/usr/local/Cellar/autoconf/2.71/share/autoconf/autoconf/trailer.m4:4: input exhausted
m4debug: input read from conftest.ac
m4debug:conftest.ac:1: input read from /usr/local/Cellar/autoconf/2.71/share/autoconf/m4sugar/foreach.m4
m4debug:/usr/local/Cellar/autoconf/2.71/share/autoconf/m4sugar/foreach.m4:362: input reverted to conftest.ac, line 1
m4debug:conftest.ac:1: input exhausted
m4trace: -1- _m4_warn([syntax], [AC_OUTPUT was never used], [])
$ /usr/local/opt/m4/bin/m4 --nesting-limit=1024 --include=/usr/local/Cellar/autoconf/2.71/share/autoconf --debug=aflqi --fatal-warning --trace=_m4_warn --reload-state=/usr/local/Cellar/autoconf/2.71/share/autoconf/autoconf/autoconf.m4f /usr/local/Cellar/autoconf/2.71/share/autoconf/autoconf/trailer.m4 conftest.ac > /dev/null
m4debug: input read from /usr/local/Cellar/autoconf/2.71/share/autoconf/autoconf/trailer.m4
m4debug:/usr/local/Cellar/autoconf/2.71/share/autoconf/autoconf/trailer.m4:5: input exhausted
m4debug: input read from conftest.ac
m4debug:conftest.ac:1: input read from /usr/local/Cellar/autoconf/2.71/share/autoconf/m4sugar/foreach.m4
m4debug:/usr/local/Cellar/autoconf/2.71/share/autoconf/m4sugar/foreach.m4:363: input reverted to conftest.ac, line 1
m4debug:conftest.ac:2: input exhausted
m4trace:/usr/local/Cellar/autoconf/2.71/share/autoconf/autoconf/trailer.m4:4: -1- _m4_warn([syntax], [AC_OUTPUT was never used], [])

(The last line in particular)

It's mostly as a result of m4_wrap, where autoconf's trailer.m4 does a m4_wrap([_AC_FINALIZE]) to dynamically insert a _AC_FINALIZE at the end of all inputs. This seems to trigger the bug in older m4 versions.

This is why other warnings are not affected. For example, the AC_USG deprecation warning works fine because that warning is thrown on the same line it is called and has nothing to do with m4_wrap/_AC_FINALIZE. The AC_OUTPUT warning is however thrown inside _AC_FINALIZE, so that's why the bug is triggered for that warning specifically.

Can it be worked around? Sure. A two-line patch to autom4te to handle such bugged traces is possible (I tried it).

Is it worth it? Probably not. I'm seeing minimal benefits of sticking with Apple's m4 at this point.

@carlocab
Copy link
Member Author

carlocab commented Mar 24, 2021

Nice, thanks for looking at it.

I just tested whether autoconf built against macOS m4 can still be used to build automake as long as brewed m4 is what's used in the automake build, and it seems that that's possible too. But, yes, not sure it's worth patching just to get working.

@Bo98
Copy link
Member

Bo98 commented Mar 24, 2021

I just tested whether autoconf built against macOS m4 can still be used to build automake as long as brewed m4 is what's used in the automake build, and it seems that that's possible too.

How autoconf is built really doesn't matter in the context of the superenv. M4 isn't invoked in the autoconf build (beyond some tests whether it is functional or not) - it's used to bake in default values to autom4te etc in the absence of a M4 env, e.g.

my $m4 = $ENV{"M4"} || '/usr/bin/m4';

So we shouldn't do anything hybrid here, for the benefit of those using it outside of the superenv.

I also suggest checking this isn't using the Cellar path or anything like that. I don't think it will be though.

@carlocab
Copy link
Member Author

carlocab commented Mar 24, 2021

I also suggest checking this isn't using the Cellar path or anything like that. I don't think it will be though.

Nope. In autom4te:

# $M4.
my $m4 = $ENV{"M4"} || '/usr/local/opt/m4/bin/m4';

and in autoupdate:

# m4.
my $m4 = $ENV{"M4"} || '/usr/local/opt/m4/bin/m4';

The only Cellar references I can find are to autoconf's Cellar path.

@fxcoudert
Copy link
Member

fxcoudert commented Mar 24, 2021

I'd be for keeping it simple, and depending on m4. Apple's version is really old, and they've consistently been telling us (python, ruby, etc) that they don't intend to update those much, and we should rely on our own versions.

Regarding superenv, I think what it's doing is trying to enforce the use of Apple m4 even when Homebrew has its own m4 installed. This made sense when autoconf didn't depend on m4, but now we're going to add the dependency, and this line should be removed.

Formula/automake.rb Outdated Show resolved Hide resolved
carlocab added a commit to carlocab/brew that referenced this pull request Mar 24, 2021
Building automake fails with autoconf 2.70+, when autoconf is used with
macOS m4. It therefore makes sense for autoconf to depend on brewed m4.

However, without the change proposed here, the setting of the M4
environment variable in superenv breaks the automake build.

Related: Homebrew/homebrew-core#73797
@carlocab carlocab force-pushed the ac-2.71 branch 2 times, most recently from 0b9707e to 161073e Compare March 24, 2021 20:22
Comment on lines 22 to 23
# TODO: Remove when https://github.com/Homebrew/brew/pull/10921
# lands in a tagged release of brew
Copy link
Member

@Bo98 Bo98 Mar 24, 2021

Choose a reason for hiding this comment

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

I think this whole PR has to wait for a tagged release, as any usage of autoconf 2.71 in other formulae relies on the M4 env being removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually think it'll work fine in those cases as long as they don't run automake's test for autoconf, but, yes, better to play it safe.

Copy link
Member

@Bo98 Bo98 Mar 24, 2021

Choose a reason for hiding this comment

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

It's a bit more than that. It affects any autoconf file that has a AC_INIT but no AC_OUTPUT. automake's test just happens to have one such file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, ok. I've managed to build some things just fine with autoconf + automake as in this PR while superenv still set M4, but I guess I was just lucky.

@carlocab carlocab added do not merge brew Issue may be Homebrew/brew related labels Mar 24, 2021
@carlocab carlocab added the license License PRs label Mar 25, 2021
@carlocab carlocab mentioned this pull request Mar 25, 2021
5 tasks
This needs to depend on brewed m4, or else building automake with
autoconf fails.

Also, update the license. Ref:

    ❯ autoconf --version
    autoconf (GNU Autoconf) 2.71
    Copyright (C) 2021 Free Software Foundation, Inc.
    License GPLv3+/Autoconf: GNU GPL version 3 or later
Remote the patch, as it is no longer needed. automake 1.16.3 now
ships with new enough config scripts.
@carlocab carlocab changed the title autoconf 2.71 autoconf 2.71 (+ automake, libtool updates) Mar 25, 2021
@carlocab
Copy link
Member Author

Updated libtool to also depend on brewed m4 (so we don't have mixed versions in the dependency tree). More context at #73932.

carlocab added a commit to carlocab/homebrew-core that referenced this pull request Mar 26, 2021
libtool should have a runtime dependency on m4, since both glibtool and
glibtoolize define functions that look for m4.

Now that autoconf will depend on brewed m4 (Homebrew#73797), libtool should too.
Otherwise, this creates a mixed version dependency.

While we're here, let's get rid of the outdated sed shim fix.
@carlocab carlocab mentioned this pull request Mar 26, 2021
5 tasks
@carlocab
Copy link
Member Author

libtool commit moved to #73981.

@carlocab carlocab changed the title autoconf 2.71 (+ automake, libtool updates) autoconf 2.71 (+ automake update) Mar 26, 2021
BrewTestBot pushed a commit that referenced this pull request Mar 26, 2021
libtool should have a runtime dependency on m4, since both glibtool and
glibtoolize define functions that look for m4.

Now that autoconf will depend on brewed m4 (#73797), libtool should too.
Otherwise, this creates a mixed version dependency.

While we're here, let's get rid of the outdated sed shim fix.

Closes #73981.

Signed-off-by: Bo Anderson <mail@boanderson.me>
Signed-off-by: FX Coudert <fxcoudert@gmail.com>
Signed-off-by: Carlo Cabrera <30379873+carlocab@users.noreply.github.com>
@carlocab
Copy link
Member Author

brew 3.0.10 has been tagged. Merging.

@BrewTestBot
Copy link
Member

:shipit: @carlocab has triggered a merge.

@carlocab carlocab deleted the ac-2.71 branch March 29, 2021 11:21
@github-actions github-actions bot added the outdated PR was locked due to age label Apr 29, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge-skip `brew pr-automerge` will skip this pull request brew Issue may be Homebrew/brew related license License PRs outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants