-
Notifications
You must be signed in to change notification settings - Fork 168
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
base: master
Are you sure you want to change the base?
Conversation
897f8f7
to
3334821
Compare
This adds the missing conditional that is needed with the default set to "as it was" PUC Lua with LuaJIT being an optional What this is still missing is a convenient way for the calling configure script to set the default the other way around. |
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? |
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. |
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? |
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. |
I wonder what is the status of this PR? It's still marked as a draft. |
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 |
I'm having trouble coming up with an established example to follow...
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. |
One possibility (the typical way, really, for a macro to provide options to the developers of a
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). |
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 |
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. |
66ace48
to
13ff011
Compare
I created a revert for the original patch in #318. I'd really like to move forward with the release. |
I've worked on this for hours and hours and only turned up problems in other macros. For example the
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). |
I think I finally found an incantation that actually keeps This uses @eli-schwartz's suggestion of a 5th argument for the developer to set the default and only becomes a provider of the The interaction that still needs testing is what happens if |
Yup -- AS_IF is intended for m4-safe generation of shell |
Soooo, yes it's true if you call this twice with conflicting options you'll get two entries in the configure flags:
$ ./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 |
cbb166b
to
9f99895
Compare
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])"], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Have you had time to consider merging either this PR or your revert, and releasing a new version that fixes the regression? |
See comments in #288