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

Fix lua configure detection #71

Closed
wants to merge 2 commits into from
Closed

Conversation

nkichukov
Copy link
Contributor

We introduce a new configure switch called --with-lua-impl=,
which is used to store user defined and yet supported lua
implementation to use. If the parameter is not specified,
lua support is not built in.

For package maintainers this is a breaking fix, as it
changes the previous --with-lua-suffix, so watch out.

The previous implementation did not work on my system, no
matter what I tried, thus the detection and configuration
is now handled via PKG_HAVE_DEFINE_WITH_MODULES, which
seems to work fine for the various implementations, tested
with luajit, lua5.1 and lua5.4.

We introduce a new configure switch called --with-lua-impl=,
which is used to store user defined and yet supported lua
implementation to use. If the parameter is not specified,
lua support is not built in.

For package maintainers this is a breaking fix, as it
changes the previous --with-lua-suffix, so watch out.

The previous implementation did not work on my system, no
matter what I tried, thus the detection and configuration
is now handled via PKG_HAVE_DEFINE_WITH_MODULES, which
seems to work fine for the various implementations, tested
with luajit, lua5.1 and lua5.4.
@garlick
Copy link
Member

garlick commented Oct 19, 2021

My ubuntu 20.04 system chokes when I run autogen.sh

configure.ac:61: error: possibly undefined macro: PKG_HAVE_DEFINE_WITH_MODULES
      If this token and others are legitimate, please use m4_pattern_allow.
      See the Autoconf documentation.

This is with pkg.m4 provided by pkg-config 0.29.1-0ubuntu4.

@garlick
Copy link
Member

garlick commented Oct 19, 2021

Also, is there a way we can do this without the "breaking fix"?

Edit: actually, maybe a good thing to do here would be for you to open an issue first, and dump in your relevant info.

@nkichukov
Copy link
Contributor Author

Here is the problem, we do not use pkg-config in this project, we already depend on pkgconf, see:
ff26333#diff-b335630551682c19a781afebcf4d07bf978fb1f8ac04c6bf87428ed5106870f5R10

It's in the readme ;-)
I will see what I can do so we fail more gracefully when pkg-config is in use instead of pkgconf. Kind of early detect and fail with proper alert message instead of what you experienced...

I've compiled this project just fine on Ubuntu 20 using pkgconf, as per the readme.

To make this a non-breaking change, we can rename --with-lua-impl to --with-lua-suffix, as long as the current --with-lua-suffix can be one of:
luajit|lua5.1|lua5.4

@garlick
Copy link
Member

garlick commented Oct 20, 2021

I think it will actually work with either, prior to this PR, which is sort of nice since you actually have to replace the default pkg-config on Ubuntu to get pkgconf. Could we try to keep that flexibility?

@nkichukov
Copy link
Contributor Author

pkgconf has become the default tool for various distributions in the last few years, GNU/Gentoo Linux including.
Redhat and Fedora:
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/GMLKQHMPHDIYXQAWC2DT6TAIOEWQIQVO/
And possibly many others.

Unfortunately, the macro from this PR relies on the version of pkg.m4 that is only shipped with pkgconf and does not exist in the one from pkg-config...

If PKG_HAVE_DEFINE_WITH_MODULES is not defined, fail early and
print more user friendly message.

Introduce dependency on pkgconf instead of pkg-config package.
@nkichukov
Copy link
Contributor Author

Hello Jim,
I've added a second commit that now checks for the presence of PKG_HAVE_DEFINE_WITH_MODULES macro from pkg.m4 and if it is not defined, in the case of someone using pkg-config instead of pkgconf, it fails early and prints a user friendly message about what needs to be done.

If the previous lua check (--with-lua-suffix) accepts the same set of parameters as the new --with-lua-impl, we can rename it to what it used to be to preserve the switch name.

Also, I've added 'set -e' at the start of autogen.sh so that it fails when a command exits abnormally, instead of running the rest when it actually should not.

Copy link
Member

@garlick garlick left a comment

Choose a reason for hiding this comment

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

Could you try going back to master and updating config/ax_lua.m4 to the latest version from savannah:
http://git.savannah.gnu.org/gitweb/?p=autoconf-archive.git;a=blob_plain;f=m4/ax_lua.m4

Docs are here:
https://www.gnu.org/software/autoconf-archive/ax_lua.html

Then apply this diff to configure.ac

diff --git a/configure.ac b/configure.ac
index 1cb8579..ca27393 100644
--- a/configure.ac
+++ b/configure.ac
@@ -68,8 +68,8 @@ X_AC_CURSES
 X_AC_CHECK_COND_LIB(munge, munge_ctx_create)
 X_AC_CHECK_COND_LIB(cap, cap_get_proc)
 X_AC_TCMALLOC
+AX_PROG_LUA([5.1],[5.5])
 AX_LUA_HEADERS
-AX_LUA_HEADERS(501)
 AX_LUA_LIBS
 X_AC_RDMATRANS
 

If you just run configure with no options, does it find your lua package?

I found we had a very old one that no longer works on modern distros. Other projects I work on had had updated around 2017(!) but diod just silently disables config file support (and tests for it) when lua is not available so we didn't notice in CI (a matter for another pr).

If this works, I prefer it because on most systems it just works without options. If not, let's spend a bit of time trying to figure out why.

@nkichukov
Copy link
Contributor Author

If no lua implementation is provided, my change will disable lua support, and I consider it as designed. I believe lua should be included only if the user has explicitly decided to include it by providing the required implementation, jit|5.1|5.4 etc.

I understand your view point regarding backwards compatibility, but I'd rather not go back to including updated ax_lua.m4 in the project. Most likely, if you do that, you will end up in the same situation in a few years from now.

Switching to pkgconf in 'master' is the way to go, if you ask me. In this way you remove the dependency of maintaining automake scripts in your project, but let pkgconf do it for you.

I do not believe pkg-config comes in by default on a new deployment of any distribution and master is all about up-to-date code. On top of my head, Redhat 8, Fedora, Arch and Gentoo Linux have all switched to pkgconf years ago so this change should not be causing any problems on any of those. People running outdated releases of non-rolling-update distros could manually install pkgconf which will replace their pkg-config or just use diod release that has not yet switched to pkgconf.

Feel free to close this PR without merging it if you insist on sticking to pkg-config for the project in the future.

This was referenced Oct 23, 2021
@garlick
Copy link
Member

garlick commented Oct 23, 2021

I want to go with the other solution, but let's make sure it fixes your specific problem.

@garlick
Copy link
Member

garlick commented Oct 24, 2021

Closing this one. Sorry we didn't argree.

@garlick garlick closed this Oct 24, 2021
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.

2 participants