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

Make LuaJIT detetction first optional for developers, secondarily for users #313

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Oct 20, 2024

See comments in #288

@alerque alerque force-pushed the luajit branch 4 times, most recently from 897f8f7 to 3334821 Compare October 20, 2024 09:59
@alerque
Copy link
Contributor Author

alerque commented Oct 20, 2024

This adds the missing conditional that is needed with the default set to "as it was" PUC Lua with LuaJIT being an optional --with-luajit configure flag away.

What this is still missing is a convenient way for the calling configure script to set the default the other way around.

@eli-schwartz
Copy link
Contributor

I'm slightly nervous about this change since the original premise of the previous patch was to integrate with people who chose to include such a configure option, and moving over to being the provider of such an option might be something not all consumers want.

Any particular reason you opted for this rather than for making it truly optional in fact?

@alerque
Copy link
Contributor Author

alerque commented Oct 20, 2024

Because I looked around at how other macros that have optional configurations like this and how they handled them and this seemed to be the most prevalent method. Also it was the code I'd already tested and had documented it was expected to work with. That being said I'm happy to adapt it so that the developer (rather than the user) has to opt in to the detection and that they can set what the default is. I just hadn't yet found any good precedent to follow for that, hence starting with this so people hitting this bug at least had something else they could drop in for now.

Note I opened the PR in draft mode because I realize the default needs some sort of toggle. If we need to toggle even exposing the option that's fine too. I just didn't have the code handy to kick off a PR with.

@peti
Copy link
Collaborator

peti commented Oct 21, 2024

I would like to release a new version of the Autoconf Archive. Should we maybe revert your previous patch to the lux macro until this fix is ready?

@alerque
Copy link
Contributor Author

alerque commented Oct 21, 2024

And then we go another year before it gets a review? Personally I'd rather just fix it to work.

@peti
Copy link
Collaborator

peti commented Oct 21, 2024

And then we go another year before it gets a review? Personally I'd rather just fix it to work.

By all means, please do.

@peti
Copy link
Collaborator

peti commented Nov 3, 2024

I wonder what is the status of this PR? It's still marked as a draft.

@alerque
Copy link
Contributor Author

alerque commented Nov 3, 2024

I started working an fixing this with a different approach to cover more use cases, but my son was in the hospital for a couple weeks when this was fresh and I haven't gotten sufficient testing in on the solutions. Releasing one of my bigger projects that uses this is top FOSS priority right now and resolving this issue is one of the last remaining issues in the milestone.

@alerque
Copy link
Contributor Author

alerque commented Nov 4, 2024

I'm having trouble coming up with an established example to follow...

  • Plenty of other macros provide relevant --with-X flags,
  • Others consume the results,
  • But none that I've found so far make it a developer-facing option to expose the user-facing configure flag or not.

In this case not only should the developer facing option (whatever that looks like) optionally enable the user facing flag, it should probably determine whether the default value is truthy or falsey.

Does a similar case come to mind? The majority of macros I looked at just unconditionally provide the configure options.

@eli-schwartz
Copy link
Contributor

eli-schwartz commented Nov 4, 2024

One possibility (the typical way, really, for a macro to provide options to the developers of a configure.ac), would be:

AX_PROG_LUA([MINIMUM-VERSION], [TOO-BIG-VERSION],
            [ACTION-IF-FOUND], [ACTION-IF-NOT-FOUND],
            [ENABLE_LUAJIT])

If the fifth option to the macro is toggling luajit on, then define the option and the AM_COND_IF.

ENABLE_LUAJIT could take values such as "default" (luajit is default-enabled), "ask" (luajit is default-disabled), "" (luajit isn't a configure choice if the fifth option to the macro isn't passed).

@eli-schwartz
Copy link
Contributor

I went into the weeds a bit trying to figure out if there's a way for an m4 macro to determine whether a previous configure.ac line has invoked AC_ARG_WITH. I couldn't find a way that didn't involve checking the contents of _AC_USER_OPTS which I am not sure is ok to do since it is underscore prefixed. (This is the m4 variable that defines one of the inner loops to AC_INIT's argument parser.)

@alerque
Copy link
Contributor Author

alerque commented Nov 4, 2024

Thanks @eli-schwartz. I hadn't realized a 5th argument was an option but that makes a lot of sense. Especially since no argument supplied can easily be used to maintain the old behavior.

I just messed around with documenting the possibilities. I'm working on implementing them not but the documentation change is posted if there is any early feedback.

@alerque alerque force-pushed the luajit branch 2 times, most recently from 66ace48 to 13ff011 Compare November 4, 2024 21:04
@peti
Copy link
Collaborator

peti commented Nov 9, 2024

I created a revert for the original patch in #318. I'd really like to move forward with the release.

@alerque
Copy link
Contributor Author

alerque commented Nov 13, 2024

I've worked on this for hours and hours and only turned up problems in other macros. For example the AX_WITH_PROG macro has a totally bogus AS_IF conditional where the inner contents runs unconditionally because the AC_ARG_WITH macro is hoisted out of context. This is even the documented behaviour and my testing shows it isn't really working.

ensures any required macros of HANDLE_FOO are expanded before the first test.

I'll PR a fix for that as soon as I actually figure out a sane way of having cake (developer facing defaults) and eating it too (allowing the user to reverse defaults if allowed by the developer).

@alerque
Copy link
Contributor Author

alerque commented Nov 13, 2024

I think I finally found an incantation that actually keeps AC_ARG_WITH from being expanded too soon before we know what the developer default is. I need to test some other environments and cleanup the history, but I think it is finally behaving as I proposed in the documentation above for early review.

This uses @eli-schwartz's suggestion of a 5th argument for the developer to set the default and only becomes a provider of the --with-luajit flag for the allow|prefer options, not never|always.

The interaction that still needs testing is what happens if AX_PROG_LUA gets called more than ones, but that might be considered a usage error anyway. The developer potentially sending mixed signals might net them multiple help strings.

@eli-schwartz
Copy link
Contributor

I've worked on this for hours and hours and only turned up problems in other macros. For example the AX_WITH_PROG macro has a totally bogus AS_IF conditional where the inner contents runs unconditionally because the AC_ARG_WITH macro is hoisted out of context. This is even the documented behaviour and my testing shows it isn't really working.

Yup -- AS_IF is intended for m4-safe generation of shell if ....; then ....; fi blocks, not for evaluating m4 conditionals to determine whether or not to expand a macro. And AC_ARG_WITH doesn't support being run inside a shell if ...; then ...; fi block as it hoists itself into the initialization preamble that loops over argument processing. I'm not surprised that would mess up -- best idea is to unconditionally run AC_ARG_WITH, and use AS_IF inside of it to determine whether VARIABLE has been set and default it to $withval. Or at least save $withval to a temporary variable and then run a non-nested AS_IF afterwards.

@alerque
Copy link
Contributor Author

alerque commented Nov 13, 2024

Soooo, yes it's true if you call this twice with conflicting options you'll get two entries in the configure flags:

AX_PROG_LUA([5.1], [], [], [], [allow])
AX_PROG_LUA([5.1], [], [], [], [prefer])
$ ./configure --help
  --without-luajit        prefer PUC Lua over LuaJIT
  --with-luajit           prefer LuaJIT over PUC Lua, even if the latter is
                          newer

I don't think this is a problem because that is indicative of a badly setup configure.ac or other macros. Also it needs exactly that pair to get added twice because the other two options never and always don't trigger a user facing config option at all. For backwards compatibility a blank value is handled as never.

@alerque alerque marked this pull request as ready for review November 13, 2024 20:37
@alerque alerque force-pushed the luajit branch 2 times, most recently from cbb166b to 9f99895 Compare November 13, 2024 21:36
@alerque
Copy link
Contributor Author

alerque commented Nov 14, 2024

I just released sile v0.15.6 with exactly the macro code in 9f99895. I'll keep an eye on distro packaging but no far so good.

m4/ax_lua.m4 Outdated
@@ -205,10 +210,36 @@ AC_DEFUN([AX_PROG_LUA],
dnl Make LUA a precious variable.
AC_ARG_VAR([LUA], [The Lua interpreter, e.g. /usr/bin/lua5.1])

dnl Figure out whether we should expose LuaJIT as a user facing configure
dnl flag and if so whether the default option should be with or without.
AS_CASE(["m4_default([$5], [never])"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably this should be an m4_case, not an AS_CASE, right?

See e.g. m4/ax_is_release.m4:

AC_DEFUN([AX_IS_RELEASE],[
    AC_BEFORE([AC_INIT],[$0])

    m4_case([$1],
      [git-directory],[
        # $is_release = (.git directory does not exist)
        AS_IF([test -d ${srcdir}/.git || (test -f ${srcdir}/.git && grep \.git/worktrees ${srcdir}/.git)],[ax_is_release=no],[ax_is_release=yes])
      ],
      [minor-version],[
        # $is_release = ($minor_version is even)
        minor_version=`echo "$PACKAGE_VERSION" | sed 's/[[^.]][[^.]]*.\([[^.]][[^.]]*\).*/\1/'`
        AS_IF([test "$(( $minor_version % 2 ))" -ne 0],
              [ax_is_release=no],[ax_is_release=yes])
      ],
      [micro-version],[
        # $is_release = ($micro_version is even)
        micro_version=`echo "$PACKAGE_VERSION" | sed 's/[[^.]]*\.[[^.]]*\.\([[^.]]*\).*/\1/'`
        AS_IF([test "$(( $micro_version % 2 ))" -ne 0],
              [ax_is_release=no],[ax_is_release=yes])
      ],
      [dash-version],[
        # $is_release = ($PACKAGE_VERSION has a dash)
        AS_CASE([$PACKAGE_VERSION],
                [*-*], [ax_is_release=no],
                [*], [ax_is_release=yes])
      ],
      [always],[ax_is_release=yes],
      [never],[ax_is_release=no],
      [
        m4_if($1, [], m4_fatal([$0: invalid policy $1. Valid policies: git-directory, minor-version, micro-version, dash-version, always, never]))
      ])
])

Instead of expanding a shell:

case never in
    never)
        default_luajit=no
        with_luajit=no
        ;;
    allow)
        defaut_luajit=no
        # m4_if([$5], [allow], ...) returned false here
        test "x$with_luajit" != 'xyes' && with_luajit=no
        ;;
    prefer)
        .........
    *)
        as_fn_error $? "Unrecognized value for ENABLE_LUAJIT"
        ;;
esac

it just emits only the code for the branch you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That seems reasonable, and saves the extra step of repeating the check with m4_if just to keep the flag help from being hoisted out twice.

Copy link
Contributor Author

@alerque alerque Nov 15, 2024

Choose a reason for hiding this comment

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

How does e801fec look, it seems to give the results I expect?

Copy link
Contributor

Choose a reason for hiding this comment

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

Any particular reason you didn't opt to use the pattern I mentioned above:

      [
        m4_if($1, [], m4_fatal([$0: invalid policy $1. Valid policies: git-directory, minor-version, micro-version, dash-version, always, never]))
      ])

and instead used:

    [
      AC_MSG_ERROR([Unrecognized value for ENABLE_LUAJIT])
    ]

Using AC_MSG_ERROR means that code is successfully generated into ./configure which unconditionally errors out -- whereas using m4_fatal would mean that autoreconf -fi can fail immediately if you pass the wrong value to AX_PROG_LUA. See e.g. 2dde1ac

Also IMHO it's slightly cleaner than the AM_CONDITIONAL([LUAJIT], [test "x$with_luajit" == "xyes"]) if you can avoid generating code at all. I wasn't sure how exactly to do this without inlining AM_COND_IF([LUAJIT], into each branch. It may be possible to do this by defining it as a private macro e.g. _LUA_FINDINTERP and calling it in each branch of m4_case, not sure if it's worth it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any particular reason [...]

Because that was a full blown example from another macro and we were chasing a different issue at that point I didn't recognize that as a relevant difference with it's own advantages.

Thanks for the explanation, and yes failing at autoconf time is better. I just implemented that pattern.

As for trying to avoid the later use of AM_CONDITIONAL I can see ways to rig it up inside of this macro (although the result would be quite a bit more convoluted and I don't see any significant gains to make it that complex), but more than that it is very useful to be able to use AM_CONF_IF outside of this macro later. So that seems like all-pain and no-gain to me.

@alerque alerque changed the title Fix LuaJIT detetction to be actually optional Make LuaJIT detetction first optional for developers, secondarily for users Nov 16, 2024
@alerque alerque requested a review from eli-schwartz November 23, 2024 08:08
@eli-schwartz
Copy link
Contributor

I created a revert for the original patch in #318. I'd really like to move forward with the release.

Have you had time to consider merging either this PR or your revert, and releasing a new version that fixes the regression?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants