Closed drok closed 4 months ago
drok
commented
5 months ago In configure.ac, why change
AX_CXX_COMPILE_STDCXX([11], [ext], [optional])frommandatorytooptionalwhen C++11 is used in the code base? https://www.gnu.org/software/autoconf-archive/ax_cxx_compile_stdcxx.html
This is a cut-and-paste error - my bad. Fixed in 6efd623
drok
commented
5 months ago Instead of [5.0.0] in configure.ac, I prefer to use the major and minor version only, which means less manual updating to do.
Instead of
PACKAGE_VERSIONin ugrep.hpp, I have to use the "5.1.1" string because I'm also building on MSVC++ without autotools.
So you want the version string in configure.ac to be different than that in visual studio? You don't care that the version number that source users see is different than the binary users?
Note the Parallel Harness commit, after which the test result headers include the version number from AC_INIT. See here for an example, or run make check
The version number is also used to determine the tarball filename, the version number in the header of the manpage, and the version number someone sees when they open your project in VSCode with my Autotools Integration extension - surely you don't want that number to be wrong?
You can have configure generate a version resource to use in visual studio like described here https://stackoverflow.com/questions/23204774/how-do-you-set-the-version-of-a-c-cli-project-in-visual-studio
Or the other way around, have visual studio generate configure.ac from a template, inserting the project version number.
In any case, consistent versioning of your project is needed; I don't think setting the version number in a source file like ugrep.hpp is right. Typically the version is project metadata, applied at compilation time via configuration; it is not part of the program proper.
genivia-inc
commented
5 months ago Instead of [5.0.0] in configure.ac, I prefer to use the major and minor version only, which means less manual updating to do. Instead of
PACKAGE_VERSIONin ugrep.hpp, I have to use the "5.1.1" string because I'm also building on MSVC++ without autotools.So you want the version string in configure.ac to be different than that in visual studio? You don't care that the version number that source users see is different than the binary users?
No, that is not what I said. I want to keep a literal version string. The versions info in configure.ac doesn't matter (much) anyway in the project as it is not used anywhere, but perhaps for package maintainers it is important. Don't know.
The project should also compile without having to use or rely on autotools. That's what is more important.
EDIT: let me add that I've used scripts with other projects to auto-update version info in source code, so they are never out of sync. We could do the same with ugrep, if the configure.ac version field must reflect the patch version number.
EDIT 2: as an example, this is the script I wrote for RE/flex to bump up the version and substitutes REFLEX_VERSION with the version string provided as an argument to the script:
#!/bin/sh
if [ "$#" = 1 ]
then
echo
echo "Bumping reflex version to $1"
sed "s/REFLEX_VERSION \"[^\"]*\"/REFLEX_VERSION \"$1\"/" src/reflex.h > src/reflex.tmp && mv -f src/reflex.tmp src/reflex.h || exit 1
# this may be needed to reconfigure for glibtoolize for example
# autoreconf -fvi
# removed, because git saves configure.ac as new causing trouble with aclocal.m4
# sed "s/AC_INIT(reflex,[^)]*)/AC_INIT(reflex,$1)/" configure.ac > configure.tmp && mv -f configure.tmp configure.ac || exit 1
./clean.sh
./build.sh || exit 1
./man.sh $1
./clean.sh
aclocal
autoheader
rm -f config.guess config.sub ar-lib compile depcomp install-sh missing
automake --add-missing --foreign
autoconf
automake
touch config.h.in
# removed, we visit GitHub releases to create a tag instead
# pushd $HOME/GitHub/RE-flex; git tag -a reflex-$1 -m "reflex version $1"; popd
else
echo "Usage: ./makemake.sh 4.v.v"
exit 1
fiEDIT 3: auto-updating configure.ac is simply adding this command to the script:
sed "s/^\(AC_INIT(\[re-flex\],\[\)[0-9.]*/\1$1/" configure.ac > configure.tmp && mv -f configure.tmp configure.ac || exit 1EDIT 4: like so in a makemake.sh script for ugrep updating:
#!/bin/sh
# if RE/flex was installed below this directory, then copy its files
# if [ -d ../reflex ]; then
# echo "Copying updated RE/flex source code files..."
# cp -r ../reflex/include/reflex/*.h include/reflex
# cp -r ../reflex/unicode/*.cpp lib
# cp -r ../reflex/lib/*.cpp lib
# cp -r ../reflex/fuzzy/fuzzymatcher.h include/reflex
# fi
# change lib/Makefile.am to use noinst_LIBRARIES
# sed -i .bak 's/lib_LIBRARIES/noinst_LIBRARIES/' lib/Makefile.am
# rm -f lib/Makefile.am.bak
if [ "$#" = 1 ]
then
echo
echo "Bumping ugrep version to $1"
sed "s/UGREP_VERSION \"[^\"]*\"/UGREP_VERSION \"$1\"/" src/ugrep.hpp > src/ugrep.tmp && mv -f src/ugrep.tmp src/ugrep.hpp || exit 1
# this may be needed to reconfigure for glibtoolize for example
# autoreconf -fvi
./build.sh || exit 1
./man.sh $1
sed "s/^\(AC_INIT(\[ugrep\],\[\)[0-9.]*/\1$1/" configure.ac > configure.tmp && mv -f configure.tmp configure.ac || exit 1
# run autoconf and automake stuff with maintainer mode disabled
aclocal
autoheader
rm -f config.guess config.sub ar-lib compile depcomp install-sh missing
automake --add-missing --foreign
autoconf
automake
touch config.h.in
./configure
else
echo "Usage: ./makemake.sh 5.v.v"
exit 1
fiLet's also add a "code base is clean" check to makemake.sh up front:
fgrep -r FIXME include lib src && echo "FIXME in code base" ; exit 1This will catch something I had forgotten to remove from the code base when actually fixed 😅
genivia-inc
commented
5 months ago Completion script generation can also be automated in makemake.sh, right after manual page generation:
./man.sh $1
pushd completions/fish ; ./compgen.sh > /dev/null ; popd || exit 1
pushd completions/zsh ; ./compgen.sh > /dev/null ; popd || exit 1Note: renaming compgen to compgen.sh is preferable, so let's rename.
drok
commented
5 months ago Note: renaming
compgentocompgen.shis preferable, so let's rename.
Renamed in eb1600b and 6ea3e86
drok
commented
5 months ago makemake.sh script for ugrep updating
This is a lot of work to avoid the built-in feature BUILT_SOURCES - you can have it generate a version.h as a built source (ie, distributed) which you add to git. This means you can use the version.h file in Visual Studio without having/depending on autotools there, while keeping the version number in sync with the AC_INIT source of truth.
The makemake.sh technique will definitely catch even experienced autotools users by surpise, as it doesn't fit the typical interface autoreconf -i && ./configure && make && make install - there is no step there for generating or patching a configure.ac file, and this hack is not described in any of the usual 'README' or 'INSTALL' files. I certainly had not noticed it until your message above, where you mention it.
Some programs that interact with your project absolutely use the version string in your AC_INIT. If you supply a version number, it should not be wrong, IMHO. I gave the example of the Autotools Integration extension for VSCode, which presents the version number, project name and issues URL from AC_INIT to users in the UI. The users will be understandably confused if VScode reports "ugrep 4.5" when they expected to see "ugrep 5.0.0"
genivia-inc
commented
5 months ago makemake.sh script for ugrep updating
This is a lot of work to avoid the built-in feature
BUILT_SOURCES- you can have it generate aversion.has a built source (ie, distributed) which you add to git. This means you can use the version.h file in Visual Studio without having/depending on autotools there, while keeping the version number in sync with the AC_INIT source of truth.
I do not use MSVC++ as my base development platform. Not sure when or how you inferred this from my comments.
I mostly use MacOS for development and also use Linux on several devices for projects and for testing portability and performance.
The
makemake.shtechnique will definitely catch even experienced autotools users by surpise, as it doesn't fit the typical interfaceautoreconf -i && ./configure && make && make install- there is no step there for generating or patching a configure.ac file, and this hack is not described in any of the usual 'README' or 'INSTALL' files. I certainly had not noticed it until your message above, where you mention it.
Nobody should ever have to use makemake.sh besides me before a release. It's just doing things in sync that other folks typically do by hand. The fact that I include these scripts is a service to others interested in this and for completeness of the project repo. Otherwise, I will just strip the repo bare and others will have to figure out how to prep a release by hand instead.
Some programs that interact with your project absolutely use the version string in your AC_INIT. If you supply a version number, it should not be wrong, IMHO. I gave the example of the Autotools Integration extension for VSCode, which presents the version number, project name and issues URL from AC_INIT to users in the UI. The users will be understandably confused if VScode reports "ugrep 4.5" when they expected to see "ugrep 5.0.0"
Again, I don't care 0% about VScode. Not using it. Neither should others. I've documented all the MSVC++ projects build stuff in ugrep/vs for you and anyone else to use, if need be.
alerque
commented
5 months ago I prefer to use the major and minor version only, which means less manual updating to do.
Sorry I've been engaged in some other work and haven't had time to follow ever detail here.
On the versioning front though I'd like to suggest there is a better way to get both more detailed versioning AND require zero maintenance. I've implemented autotools hooks for versioning from Git that set the version string relative to release tags, usable without intervention from from either Git clones or source dist packages (make dist) (not git archive generated tarballs). No source files to edit, ever, and every source and binary build has either the exact tag release for releases or additional counter info for commits between releases.
You can see this working on projects like vcsh, bzip3, git-warp-time, sile, and many more.
If no manual-updating at all sounds good I'll try to add a PR into this one.
genivia-inc
commented
5 months ago I prefer to use the major and minor version only, which means less manual updating to do.
Sorry I've been engaged in some other work and havn't had time to follow ever detail here.
On the versioning front though I'd like to suggest there is a better way to get both more detailed versioning AND require zero maintenance. I've implemented autotools hooks for versioning from Git that set the version string relative to release tags, usable without intervention from from either Git clones or source dist packages (
make dist) (notgit archivegenerated tarballs). No source files to edit, ever, and every source and binary build has either the exact tag release for releases or additional counter info for commits between releases.You can see this working on projects like
vcsh,git-warp-time,sile, and many more.If no manual-updating at all sounds good I'll try to add a PR into this one.
I appreciate your involvement and suggestions.
That's very neat and good for you.
But personal use cases are not transferrable to others who might prefer different scenarios to work on projects and release updates. I am very well aware what others do.
I want to make sure it is always possible to have zero dependence on Git and zero dependence on autotools as a fallback. I learned my lessons in the past. Everything should be buildable, maintainable and updatable without requiring or using git and autotools. Use git and autotools only as support to release and install the project. This means I won't take a tag from git to update the version field. Likewise, I won't take the AC_INIT version field to expand in the code. The code base is the boss, not the environment that supports releasing and installing it.
alerque
commented
5 months ago I want to make sure it is always possible to have zero dependence on Git and zero dependence on autotools as a fallback.
Did I say either of those would become requirements to build using my versioning suggestion? They aren't. I said the versioning would work either in a Git clone (in that case git is expected yes) or in a source distribution (in which case git is not required or even used. I didn't even mention there are several ways to "fallback" in case you wanted to override the version for whatever reason (an env var, a configure flag, or a version file). I also mentioned the disadvantage being more limited in the case of artifacts generated by git archive, but you've gone for something that isn't even a problem as not commented on how that does or doesn't fit your workflow preferences.
As for autotools, it should be a requirement to need it to build from a Git clone or git archive (where autoreconf -i should be used to transform configure.ac that is tracked in VCS in the maintainable form into configure which users run but is not tracked in VCS), but should not be required to bulid from a source package (where configure is already prebuilt and other transformations have already been made). That's what make dist generates: a tarball that has all the autotools stuff and macros expanded with all the platform specific stuff in place so that people can skip straight to ./configure with no dependency on autotools itself.
genivia-inc
commented
5 months ago https://github.com/Genivia/ugrep/pull/370/commits/ebe6400c7b19e92176539f0ba93f1a58397b5b64
Two problems with this.
The logic is wrong as I've pointed out i.e. --with-bzip3 should check to link bzip3 explicitly. Bzip3 is not linked by default, unlike the other libs.
So I patched it like this:
AC_ARG_WITH(bzip3,
[AS_HELP_STRING([--with-bzip3],
[Use the bzip3 library when installed to decompress .bz3 files])],
[with_bzip3_library="$withval"],
[with_bzip3_library=""])
AS_IF([test "x$with_bzip3_library" != "x"],[
PKG_CHECK_MODULES([BZIP3], [libbzip3],
[AC_DEFINE([HAVE_LIBBZIP3], [1], [Use libbzip3 compression library])
CXXFLAGS="$CXXFLAGS $BZIP3_CFLAGS"
LIBS="$LIBS $BZIP3_LIBS"
],
# libbzip3 is optional by default, but required if --enable-bzip3 is given
[AS_IF([test -z "$enable_bzip3" ],
[AC_MSG_WARN([libbzip3 not found (optional): ugrep option -z cannot search .bz3 files])],
[AC_MSG_ERROR([libbzip3 not found. Is the devel package installed?])],
)]
)
])But this PKG_CHECK_MODULES fails to detect bzip3:
checking for libbzip3... no
configure: WARNING: libbzip3 not found (optional): ugrep option -z cannot search .bz3 fileswhen it worked before:
checking if bzip3 is wanted... yes
checking for bz3_new in -lbzip3... yes
checking for libbz3.h... yesAlso, note that I really like to do this for all libraries that are not required, so I do not need to pollute the Makefile.am with these XYZ_LIBS all over the place:
CXXFLAGS="$CXXFLAGS $BZIP3_CFLAGS"
LIBS="$LIBS $BZIP3_LIBS"Perhaps CPPFLAGS might be OK too. Just drafting something up here.
genivia-inc
commented
5 months ago https://github.com/Genivia/ugrep/pull/370/commits/6978c50c0d60226a076ea1ff78ea4d34ed1be1c4
This is incomplete. We can't disable pcre2 this way and try boost.
To do this properly:
AC_ARG_ENABLE([pcre2],[
--enable-pcre2 Use PCRE2 for option -P
@<:@defaults to enabled if PCRE2 is installed@:>@
--disable-pcre2 Do not use PCRE2])
AS_IF([test "$enable_pcre2" != "no"],[
PKG_CHECK_MODULES([PCRE2], [libpcre2-8],
[AC_DEFINE([HAVE_PCRE2], [1], [Use PCRE2 (Perl Compatible Regular Expression Library)])
have_pcre2=yes
CXXFLAGS="$CXXFLAGS $PCRE2_CFLAGS"
LIBS="$LIBS $PCRE2_LIBS"
],
[AC_MSG_RESULT(no)]
)
])
if test "${have_pcre2}" != "yes" ; then
# Check for libboost-regex library and header, adds
AC_SEARCH_LIBS([regcompA], [boost_regex boost_regex-mt], [
# Check for boost regex header
AC_CHECK_HEADER([boost/regex.hpp], [
BOOST_REGEX_FOUND=true
AC_DEFINE([HAVE_BOOST_REGEX], [1], [Use Boost Regex library])
AC_DEFINE([BOOST_REGEX_STANDALONE], [1], [Signal to boost lib that it is being used without the rest of boost])
],
[AC_MSG_WARN([PCRE2 not found and Boost Regex.hpp not found])
])
],
[AC_MSG_WARN([PCRE2 not found and Boost Regex.hpp not found])
])
fihttps://github.com/Genivia/ugrep/pull/370/commits/2fd7710e88caf164acb99f4ed119236bae87b74c
There is a mistake. The have_libz=yes var is set, but not used in the test below it. It libz is not available, then none of the other compression libraries should be linked. They all become defunct.
The test below it should be:
if test "${have_libz}" = "yes" ; thenhttps://github.com/Genivia/ugrep/pull/370/commits/8a4797188d2013252f0bfca11c49875ed079e270 https://github.com/Genivia/ugrep/pull/370/commits/9423c6bf1047d99906b5eea975ffb0e81a2c0ccc
These suggested changes do not produce checking ... and then report no with a note the the library is recommended. I really like to see a note with recommended here.
https://github.com/Genivia/ugrep/pull/370/commits/a27012a08a452fcd49fc82215ed10874ebf65350 https://github.com/Genivia/ugrep/pull/370/commits/4d2b26d838184ed33bbea37d4e3ce34d7c373e3b https://github.com/Genivia/ugrep/pull/370/commits/b596048a4c7ff75e98f1a074655fc11874e0ada4
These suggested changes do not produce checking ... and then report no with a note the the library is optional. I really like to see a note with the checking result.
I will make the changes on my end if this is too much work for you. I've already done most of the work the last hour or so while verifying the PR. I see too many problems so far and I've explained why.
genivia-inc
commented
5 months ago The decision to use PKG_CHECK_MODULES is questionable:
https://stackoverflow.com/questions/10220946/pkg-check-modules-considered-harmful
Can you explain why you believe it is superior to AC_CHECK_LIB like the ones in the m4 files in ugrep/m4?
Also boost and bzip2 checking parts in the PR configure.ac aren't using PKG_CHECK_MODULES. Using PKG_CHECK_MODULES does not appear to work as I've tried in different ways. These use the more conventional AC_SEARCH_LIBS and AC_CHECK_HEADER.
drok
commented
5 months ago These suggested changes do not produce checking ... and then report no with a note the the library is recommended. I really like to see a note with recommended here.
IMHO, recommendations belong in a place that humans read, and there is a designated place to document such things. See https://www.gnu.org/prep/standards/standards.html#Releases
I have pointed out the need for an INSTALL file several times already.
The output of the configure command is not commonly read by people, unless diagnosing/debugging (ie, when something has already gone wrong). It's not where people expect to find installation documentation.
People typically expect to find information and explanations in README, INSTALL, RELEASE-NOTES.txt and other such files written by another person. Not everyone reads documentation, but those who do expect to find it where documentation is supposed to go.
I should mention that when/if you decide that any of this PR is useful, you don't have to take the proposals as-is; you can edit your merge before committing, and in that edit you can choose to keep any, all or none of the proposed changes. What you keep and what you don't is at your discretion, you don't need to convince me to change my proposal.
My proposal reflects what I believe is the best uninterested advice I can give you, and that is the only advice I intend to give you. I don't intend to give you any less than my best advice. I have taken your feedback into consideration and will continue to do so, and amend my proposal where I made a mistake. You've already pointed out a few genuine mistakes, for which I am grateful, and I have fixed my proposal with "!fixup" commits.
However, where your feedback lacks merit, or where it's based on personal preference rather than reason, and where your preference conflicts with documented best practices or my experience, you cannot reasonably expect that I adopt your preference as my best advice. I have adopted your preference in places where it doesn't conflict with best practices (eg, renaming compgen to compgen.sh) or lessens the quality of my work.
That being said, I would like to reiterate, what I give you is merely a proposal, and you are entitled to reject it in its entirety, accept it in its entirety, or accept any part of it that you like at your own discretion.
drok
commented
5 months ago The decision to use
PKG_CHECK_MODULESis questionable: https://stackoverflow.com/questions/10220946/pkg-check-modules-considered-harmfulCan you explain why you believe it is superior to
AC_CHECK_LIBlike the ones in the m4 files in ugrep/m4?
My pleasure.
The .pc file published by library maintainers/authors contains information about compilation requirements of the library such preprocessor options (includedir), compiler options (cflags), optimization options, linker options, as well as a list of -l <library> options to be used at link time (eg, see openssl library: pkg-config --libs openssl recommends _LIBS contain -lssl -lcrypto). This allows the authors to change their dependencies or optimizations from one release to the next, as they see fit; their compile specifications travel with the library, and are consistent with each other.
The AC_CHECK_LIB method is not capable of inferring optimization flags or additional dependencies. With that method, the list of compile flags and dependencies must be known a priori by the coder who writes down the AC_CHECK_LIB check.
Some libraries do not come with pkg-config files (ie, the author has not communicated any compile time requirements, or even a name by which their library should be known; eg, the boost authors' choice of library name, 'boost_regex-mt' was not adopted by the Debian maintainers, and thus the library is named boost_regex in their distro). In those cases, library users (other developers who want the functionality), must guess the library name, compile flags and extra dependencies. Guessing means that some of the time, on some distros, the guess is wrong.
Case in point: the OpenSSL project publishes a .pc file. Their project name is openssl on every distro. That is the name you can use to reliably find the pc file, anywhere. The openssl "package" does not contain any libraries, but depends on two other packages, "libssl" and "libcrypto" - the fact that these package names are the same as the library names they contain is half coincidence, half creativity. The packages "libssl" also depends on the package "libcrypto". In turn, each of these packages includes a .pc file which lists the library name to be added to the linker command line. It is a library name that distro maintainers can change to anything they please; it can be reliably found with pkg-config openssl on every distro that includes openssl. The name 'openssl' is invariant, while the library name is arbitrary, although it doesn't usually vary.
Library authors who do publish a .pc file have the ability to pick a package name (not to be confused with library name) by which their lib will be known, and which is highly likely to be adopted by distro maintainers, unless it conflicts with another lib. They also can communicate ABI, optimizations, and other compile-time and link-time options to their users, as well as retain the ability to change these flags as they see necessary in the future, with the knowledge that their changes will propagate reliably through the supply chain to their users.
AC_CHECK_LIB don't have a supply chain to propagate flags through. The library author that relies on the users using AC_CHECK_LIB are limited to using only well-known flags (ie, none) and extra dependencies (none), and don't have the freedom to change them in future versions, lest they break existing detection scripts.
pkg-config implements disciplined propagation of metadata, while AC_CHECK_LIB implements guessing.
Turning to the stackoverflow question - the question itself contains a very good use case - the GTK library with an intricate set of dependencies. The pkg-config file reliably finds a complete, correct list of dependencies, while an AC_CHECK_LIB equivalent would be a smorgasbord of guesses.
The accepted answer is poorly reasoned because:
/p/a/t/h and then expects that pkg-config should be able to find it without setting PKG_CONFIG_PATH - it does not explain how pkg-config can be expected to know the non-standard path the user chose. There is no magic in the process, but the answer giver seems to expect that some magic will occur that makes in unnecessary to set PKG_CONFIG_PATHLDFLAGS and is able to make inferences from it. He says that running configure with LDFLAGS=-L/p/a/t/h should tell pkg-config that a .pc file can be found at /p/a/t/h - this demonstrates a fundamental lack of understanding of the purpose of LDFLAGS, and the fact that library files (.so) aren't typically, nor do they need to be installed in the same location as package manifest files (.pc). For greater certainty, LDFLAGS is meant to provide linker customizations (see ld --help), while -L/p/a/t/h together with library name, is a specification of what the input is, rather than how linking should be done (ie, inputs are not the same as options)The autotools are not a package management system which is true, but he seems to believe that pkg-config is part of autotools, and it is not. pkg-config is a separate program that happens to be commonly used with autoconf, just as a C compiler, awk, and sed and sh are commonly used with autoconf. In fact, the autoconf program does depend on pkg-config. They do happen to both be classified as 'developer tools', and thus distros typically install them together. This does not make autotools a package management system, just as it doesn't make them a compiler, a stream editor/text processor, or a shell management system.The accepted answer is opinionated and shows misconceptions about the ecosystem. In this case, his opinions seem plausible to someone who is uninformed, and thus he has a number of 'followers', but it would not pass the scrutiny of someone who is informed, as some of the commenters.
genivia-inc
commented
5 months ago The output of the configure command is not commonly read by people, unless diagnosing/debugging (ie, when something has already gone wrong). It's not where people expect to find installation documentation.
I agree from a package maintainer's point of view. But when manually installing the package and to develop and maintain the software now and in the future by me or anyone who isn't just interested in unpacking and installing the software, these messages we get from configure are useful. Otherwise, what's the point of configure to produce messages? Real warnings and errors should be distinguishable from checking notes, i.e. AC_MSG_NOTICE for a note that something entirely optional is not included makes sense I would think (which is silenced when need be) versus AC_MSG_WARN when a recommended component is not included versus AC_MSG_ERROR when a component is missing. There is a preferential difference in the usage of libraries by ugrep.
However, where your feedback lacks merit, or where it's based on personal preference rather than reason, and where your preference conflicts with documented best practices or my experience, you cannot reasonably expect that I adopt your preference as my best advice. I have adopted your preference in places where it doesn't conflict with best practices (eg, renaming compgen to compgen.sh) or lessens the quality of my work.
I have no personal preferences wrt. to autotools. But changes in the logic that change the original logic flow and result need correcting, whether these changes are in scripts or in the C++ code base. Changes to improve the C++ code base can be opened separately as issues when there is a potential problem, but not be part of a PR to overhaul autotool use. It also makes it much harder to review the PR.
Perhaps things in the PR may make your life easier, but not all changes are a win-win. For example, using makefiles for static data (man page, compgen, patterns) makes my work miserable having to juggle more moving parts in the project. The 7 points lay that out. More moving parts and dependent stuff spread out across Makefiles increases fragility.
So I wonder why PKG_CHECK_MODULES is controversial (well, at least a couple of years ago). It is also not consistently used in your PR, where in some cases AC_SEARCH_LIBS is also used. If AC_SEARCH_LIBS is safer bet (orAC_CHECK_LIB perhaps), then we should go with that, no?
NOTE: this was cross-posted with the post above that I did not see, which answers part of these questions.
drok
commented
5 months ago It is also not consistently used in your PR, where in some cases
AC_SEARCH_LIBSis also used. IfAC_SEARCH_LIBSis safer bet (orAC_CHECK_LIBperhaps), then we should go with that, no?NOTE: this was cross-posted with the post above that I did not see, which answers part of these questions.
My post on the topic of PKG_CHECK_MODULES addresses part of the questions, as you note, but for greater certainty, let me directly answer your question on why PKG_CHECK_MODULES is not used consistently.
As I mentioned in the PKG_CHECK_MODULES response, some packages do not provide a .pc file, so it is not possible to use PKG_CHECK_MODULES with them. The following of your dependencies fall in this category (these are library names):
The other dependencies do supply a .pc file, so PKG_CHECK_MODULES is used to discover their coordinates (these are package names):
The choice to use PKG_CHECK_MODULES or AC_SEARCH_LIBS was not discretionary, but was dictated by whether the respective software is distributed with a .pc manifest file or not.
genivia-inc
commented
5 months ago https://github.com/Genivia/ugrep/pull/370/commits/b4ba7e5ec0cace0c40bc7437f447ff56a0ec6366
This does not appear to detect NEON on MacOS M1.
test "x$ax_cv_support_neon_ext" = "xyes"is false. Looking into ax_ext.m4, there are arm) and aarch64) arms that differ where arm) sets ax_cv_support_... but aarch64) does not, which is where MacOS "lives".
As I mentioned before, NEON detection means NEON/AArch64 detection and it should and can work on all ARM-based CPUs that support NEON instructions.
drok
commented
5 months ago is false. Looking into
ax_ext.m4, there arearm)andaarch64)arms that differ wherearm)setsax_cv_support_...butaarch64)does not, which is where MacOS "lives".
You're right. My apologies for the mistake. Fixed in df64e49
genivia-inc
commented
5 months ago You've not included this new line for aarch64)
eval ax_cv_support_${ac_instr_acvar}_ext=yesHowever, now when NEON is detected on MacOS, an old friend warning pops up that I squashed in the past in my config code:
clang: warning: argument unused during compilation: '-mfpu=neon' [-Wunused-command-line-argument]That's why the NEON detection in my code has three different stages to include or not certain flags, in the case of MacOS it detects that -DHAVE_NEON suffices which simply enables the NEON supported intrinsics in ugrep.
genivia-inc
commented
5 months ago To experiment a bit around with this ax_ext.m4 stuff, I'm using the following which I've adapted from your PR, without having to change anything to my makefiles and code base:
AC_ARG_ENABLE(simd,
[AS_HELP_STRING([--disable-simd],
[disable SIMD CPU extensions (SSE2, AVX, NEON, etc)])],
[with_no_simd="yes"],
[with_no_simd="no"])
if ! test "$with_no_simd" = "yes"; then
# Temporarily set cross_compiling mode to detect if the compiler supports
# AVX2 / AVX512 but not check the compiling CPU
save_cross_compiling=$cross_compiling
cross_compiling=yes
# Tell AX_EXT that the CPU supports it, because runtime will check.
ax_cv_have_avx2_cpu_ext=yes
ax_cv_have_avx512bw_cpu_ext=yes
ax_cv_have_sse2_cpu_ext=yes
ax_cv_have_avx2_ext=yes
ax_cv_have_avx512bw_ext=yes
ax_cv_have_sse2_ext=yes
ax_cv_have_neon_ext=yes
AX_EXT
cross_compiling=$save_cross_compiling
fi
if test "$ax_cv_support_sse2_ext" = "yes"; then
if test "$ax_cv_support_avx2_ext" = "yes"; then
if test "$ax_cv_support_avx512bw_ext" = "yes"; then
SIMD_FLAGS="-msse2 -DHAVE_AVX512BW"
SIMD_AVX2_FLAGS="-mavx2"
SIMD_AVX512BW_FLAGS="-mavx512bw"
else
SIMD_FLAGS="-msse2 -DHAVE_AVX2"
SIMD_AVX2_FLAGS="-mavx2"
fi
else
SIMD_FLAGS="-msse2 -DHAVE_SSE2"
fi
else
if test "$ax_cv_support_neon_ext" = "yes"; then
# FIXME does not work on MacOS that we know rejects -mfpu=neon that e.g. ARM7 needs
SIMD_FLAGS="-mfpu=neon -DHAVE_NEON"
fi
fiNote that SIMD_FLAGS="-msse2 -DHAVE_SSE2" is what I want when SSE2 is detected, i.e. all code is compiled with -msse2 and intrinsics are compiled by setting -DHAVE_SSE2. That is the gist of it.
Removing this part:
if test "$ax_cv_support_neon_ext" = "yes"; then SIMD_FLAGS="-mfpu=neon -DHAVE_NEON" fiworks with MacOS because
SIMD_FLAGSis set inax_ext.m4. I'll check other machines.
EDIT
Actually, no, it does not work on MacOS either (made a mistake when testing the above quoted part)
No success on ARM Cortex-A72 CPU (ARM7), no NEON detection.
No success on ARM7 RPi 3, no NEON detection.
On Intel x64 MacOS with AVX2, ax_ext.m4 incorrectly detects AVX512BW and the script I show above sets -msse2 -DHAVE_AVX512BW. The runtime detection of AVX512BW fails in ugrep (as should be) so it fails back to AVX2 as intended. But AVX512BW should not be detected by ax_ext.m4.
genivia-inc
commented
5 months ago It looks like using CPUID alone in ax_ext.m4 to detect SIMD extensions does not work as advertised.
Furthermore, using CPUID alone is also incomplete, because the compiler may not support SIMD intrinsics. In that case the build will fail. It could fail because of a missing header file, such as immintrin.h for example.
By contrast, compiling and running a short program that tests the presence and execution of SIMD intrinsics is more robust. That's one reason why I wrote these checks in the first place.
I will investigate and experiment a bit more for the 7 point battle plan, in particular points 6 and 7. I may end up rewriting part of my own code with solutions that actually work.
genivia-inc
commented
5 months ago I don't know how folks and package maintainers invoke ./configure to cross-compile binaries that benefit from SIMD CPU extensions. Turning all SIMD CPU extensions off is really bad and unnecessary. Therefore, enabling them is important.
We should check for $cross_compiling. I assume that when cross-compiling, configure should be run with e.g. --enable-sse2 or --enable-neon to enable the corresponding SIMD CPU extensions?
genivia-inc
commented
5 months ago Updating configure.ac is done, except for cross compilation checks (see below).
Everything is working and verified on my end in an updated configure.ac. No other changes are needed, except some minor mostly cosmetic improvements to the makefiles.
NEON is now accurately detected and compiles properly on ARM6 (no NEON), ARM7 and Android ARM-A72, MacOS M1. The checks perform compile-time checks (without the dreaded -march=native) and also runtime execution checks.
Now also the SSE2/AVX2/AVX512BW checking is performed without the dreaded -march=native using a compile-time and runtime check.
The external m4's are reduced to these four, by using PKG_CHECK_MODULES when possible:
ax_boost_regex.m4
ax_check_bzip3lib.m4
ax_cxx_compile_stdcxx_11.m4
ax_pthread.m4
I recommend to remove the failing ax_ext.m4 and ax_gcc_ files. These are useless and do not work as documented.
Last step is to handle cross compilation:
I don't know how folks and package maintainers invoke
./configureto cross-compile binaries that benefit from SIMD CPU extensions. Turning all SIMD CPU extensions off is really bad and unnecessary. Therefore, enabling them is important.We should check for
$cross_compiling. I assume that when cross-compiling, configure should be run with e.g.--enable-sse2or--enable-neonto enable the corresponding SIMD CPU extensions?
Any suggestions? This is what I want to do:
if ! test "$with_no_simd" = "yes"; then
if test "x$cross_compiling" = "xyes"; then
# FIXME should we listen to --enable-sse2 / --enable-avx2 / --enable-avx512bw / --enable-neon ???
AC_MSG_WARN([cross compiling])
else
... the current SSE2/AVX2/AVX512BW checking ...Ah. I figure it out for the missing part to handle cross compilation to detect SIMD CPU extensions. It's rather trivial. Tested and verified with ARM6, ARM7 and AArch64 devices and an Intel CPU.
if test "x$cross_compiling" = "xyes"; then
# cross compiling to a host CPU, check support for SIMD CPU extension compile flags
case $host_cpu in
i[[3456]]86*|x86_64*|amd64*)
AX_CHECK_COMPILE_FLAG([-msse2],
[msse2_ok=yes],
[msse2_ok=no])
if test "x$msse2_ok" = "xyes"; then
AX_CHECK_COMPILE_FLAG([-mavx2],
[mavx2_ok=yes],
[mavx2_ok=no])
if test "x$mavx2_ok" = "xyes"; then
AX_CHECK_COMPILE_FLAG([-mavx512bw],
[mavx512bw_ok=yes],
[mavx512bw_ok=no])
if test "x$mavx512bw_ok" = "xyes"; then
SIMD_FLAGS="-msse2 -DHAVE_AVX512BW"
SIMD_AVX2_FLAGS="-mavx2"
SIMD_AVX512BW_FLAGS="-mavx512bw"
else
SIMD_FLAGS="-msse2 -DHAVE_AVX2"
SIMD_AVX2_FLAGS="-mavx2"
fi
else
SIMD_FLAGS="-msse2 -DHAVE_SSE2"
fi
fi
;;
armv5*|armv6*)
;;
arm*)
AX_CHECK_COMPILE_FLAG([-mfpu=neon],
[SIMD_FLAGS="-mfpu=neon -DHAVE_NEON"],
[SIMD_FLAGS=""])
;;
aarch64*)
SIMD_FLAGS="-DHAVE_NEON"
;;
esacNote that we expect x86/x64 machines to support SSE2, which is reasonable. Also note that we compile all the way up to AVX2 and AVX512BW, which will be selected at runtime based on CPUID of the machine on which ugrep runs.
I wonder what ax_ext.m4 is trying to do with arm). It's clearly wrong, because arm) will not match specific ARM hosts and we cannot even distinguish ARM6 from ARM7 with ARM7 supporting NEON. The ax_ext.m4 is trash IMO.
EDIT: AX_CHECK_COMPILE_FLAG is not (typically) installed, we can use AC_COMPILE_IFELSE instead like so:
# cross compiling to a host CPU: check support for SIMD CPU extension compile flags
case $host_cpu in
i[[3456]]86*|x86_64*|amd64*)
# enable SSE2/AVX2/AVX512BW extensions
if ! test "x$with_no_sse2" = "xyes"; then
AC_MSG_CHECKING([whether ${CXX} supports SSE2 intrinsics])
save_CXXFLAGS=$CXXFLAGS
CXXFLAGS="-msse2"
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <emmintrin.h>]], [[__m128i n = _mm_set1_epi8(42);]])],
[msse2_ok=yes],
[msse2_ok=no])
CXXFLAGS=$save_CXXFLAGS
AC_MSG_RESULT($msse2_ok)
if test "x$msse2_ok" = "xyes"; then
SIMD_FLAGS="-msse2 -DHAVE_SSE2"
if ! test "x$with_no_avx2" = "xyes"; then
AC_MSG_CHECKING([whether ${CXX} supports AVX2/AVX512BW intrinsics])
save_CXXFLAGS=$CXXFLAGS
CXXFLAGS="-mavx512bw"
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <immintrin.h>]], [[__m512 n = _mm512_set1_epi8(42); (void)_mm512_cmpeq_epi8_mask(n, n);]])],
[mavx_ok=yes],
[mavx_ok=no])
if test "x$mavx_ok" = "xyes"; then
SIMD_FLAGS="-msse2 -DHAVE_AVX512BW"
SIMD_AVX2_FLAGS="-mavx2"
SIMD_AVX512BW_FLAGS="-mavx512bw"
else
CXXFLAGS="-mavx2"
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <immintrin.h>]], [[__m256i n = _mm256_set1_epi8(42); (void)_mm256_movemask_epi8(_mm256_and_si256(n, n));]])],
[mavx_ok=yes],
[mavx_ok=no])
if test "x$mavx_ok" = "xyes"; then
SIMD_FLAGS="-msse2 -DHAVE_AVX2"
SIMD_AVX2_FLAGS="-mavx2"
fi
fi
CXXFLAGS=$save_CXXFLAGS
AC_MSG_RESULT($mavx_ok)
fi
fi
fi
;;
armv5*|armv6*)
;;
arm*)
# enable arm >= 7 neon extensions with option -mfpu=neon
if ! test "x$with_no_neon" = "xyes"; then
AC_MSG_CHECKING([whether ${CXX} supports ARM NEON/AArch64 intrinsics])
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <arm_neon.h>]], [[uint64x2_t n; uint64_t m = vgetq_lane_u64(n, 0);]])],
[mneon_ok=yes],
[mneon_ok=no])
if test "x$mneon_ok" = "xyes"; then
SIMD_FLAGS="-DHAVE_NEON"
else
save_CXXFLAGS=$CXXFLAGS
CXXFLAGS="-mfpu=neon"
AC_PREPROC_IFELSE([AC_LANG_PROGRAM([[#include <arm_neon.h>]], [[uint64x2_t n; uint64_t m = vgetq_lane_u64(n, 0);]])],
[mneon_ok=yes],
[mneon_ok=no])
if test "x$mneon_ok" = "xyes"; then
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <arm_neon.h>]], [[uint64x2_t n; uint64_t m = vgetq_lane_u64(n, 0);]])],
[mneon_ok=yes],
[mneon_ok=no])
if test "x$mneon_ok" = "xyes"; then
SIMD_FLAGS="-mfpu=neon -DHAVE_NEON"
else
# this is iffy,,,
CXXFLAGS="-march=native -mfpu=neon"
AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[#include <arm_neon.h>]], [[uint64x2_t n; uint64_t m = vgetq_lane_u64(n, 0);]])],
[mneon_ok=yes],
[mneon_ok=no])
if test "x$mneon_ok" = "xyes"; then
SIMD_FLAGS="-march=native -mfpu=neon -DHAVE_NEON"
fi
fi
fi
CXXFLAGS=$save_CXXFLAGS
fi
AC_MSG_RESULT($mneon_ok)
fi
;;
aarch64*)
# enable AArch64 neon extensions
if ! test "x$with_no_neon" = "xyes"; then
SIMD_FLAGS="-DHAVE_NEON"
fi
;;
esacFound a problem with Boost Regex check a couple of days ago, but didn't report it.
When using Boost Regex installed with Homebrew on MacOS in /opt/homebrew/lib/libboost_regex-mt.{a,dylib} which is the default path of Brew, this code will not find it:
# Check for libboost-regex library and header, adds
AC_SEARCH_LIBS([regcompA], [boost_regex boost_regex-mt], [
# Check for boost regex header
AC_CHECK_HEADER([boost/regex.hpp], [
BOOST_REGEX_FOUND=true
AC_DEFINE([HAVE_BOOST_REGEX], [1], [Use Boost Regex library])
AC_DEFINE([BOOST_REGEX_STANDALONE], [1], [Signal to boost lib that it's being without the rest of boost])
]but this does:
if test "x$have_pcre2" != "xyes"; then
# PCRE2 was not found: check for alternative Boost Regex library and header
AX_BOOST_REGEX
AC_DEFINE([BOOST_REGEX_STANDALONE],[1],[Signal to boost lib that it is being used without the rest of boost])
fiwhere AX_BOOST_REGEX is defined in ax_boost_regex.m4.
Autotools usage is brought to 'textbook' standards, following closely the usage recommended by the autoconf/automake Fine Manuals.
Refactorings
There are many refactorings, some of which could be standalone, but the entire package is more than the sum of its parts. The big picture:
make distcheckpasses - ie, a valid, complete tarball source distribution is producedCareful review needed
Suggested further work
The matcher.cpp file should be split into architecture-dependent compilation units.
The ugrep.exe binary should be removed from the repo. This binary can be built on Linux using cross-compilation. The pcre2 library supports cross-compilation out of the box.
References
I have found many of the the problems here using the Autotools Integration for VSCode (an extension I wrote), which was instrumental in making this depth of change easy and predictable.
This work completes what I intended to do for Genivia/ugrep#339