ugrep
ugrep copied to clipboard
Genivia
Overhaul GNU Autotools usage
I first noticed originally packaging this for Arch that the Autotools stuff was a bit wonky but let it go and moved on. With v4.4.0 breakage (#335) I had a closer look and there are several things that are a bit ... sideways. The version stuff isn't handled right, the VCS tracked bits don't work properly between releases, the tooling like make distcheck that should catch packaging issues early just doesn't work, there are some shell scripts that echo some of their commands but not others making debugging a bit of a goose chase, and likely other bugaboos.
I don't expect everybody to be an Autotools expert (and only much pain has made me even half-way competent with it). This issue is actually specifically so if the author would like to see these issues fixed they can assign the issue to me and I'll get to it sometime in my free time. No promises about when that is, just that it will be on my list.
An update 4.5 will be released soon with some additions to the configure and makefile scripts.
See #185 for 7z archive search, which required adding an lzma Makefile to build a small part of the LZMA SDK as a static library to link with ugrep.
I also made some minor changes in 4.5 to configure.ac so that when zlib is not available or not wanted then we don't link all the other compression libraries which we don't use, since option -z is disabled.
Update 4.5 is released. The project autotool files won't be updated for a while now (by me).
@alerque Do you have updates to share? If not, I'll close this issue until revisited with an update. Or I'll streamline a few things in autotools usage for upcoming releases. Nothing appears to be broken at this point.
@genivia-inc - kindly don't close this issue, even if @alerque has not had a chance to make progress.
I am working on a vscode extension integration autotools into the editor, and one of the features is diagnostics for autotools build configurations. I've been using this project along with a few others in my testbed, as a source of common errors to test the diagnosis code.
There are real problems to be solved in the ugrep configuration, and closing this issue would only sweep them under the rug.
I plan on submitting a more detailed account of the issues I have found, and proposed fixes (PR) once the upcoming pre-release version 0.2 of my extension is published. I am currently testing this release. Here is a preview of one issue and suggested fixes as produced by my extension:
Kindly allow me about a week or so to release my extension and then give you my feedback here.
I'd also like it to stay on my list. I actually started some work locally but it's easier to remember to come back to until it's done if there is an open and assigned issue. There really is some breakage lurking in here that could be fixed with proper usage of autotools.
"Proper usage of autotools"? Please don't (overdo it). You will get into trouble, guaranteed. There are many pitfalls. I've only used autotools for the past 25 years with programming experience starting in the early 80s writing BASIC, Pascal and Z80 assembly. But got bored with that, not fully understanding the power of algorithms and theory, so pursued an academic career in CS to beef that up a bit, eventually becoming department chair, which was even more "boring" (in a way), especially when a lot of "smart" people are wrong most of the time. Holding back not to criticize them or correct them was my best political move, even though some felt compelled to try to correct me when they are provably wrong or misunderstood the problem altogether. Learn and move on. Don't let ego get in the way of being productive and helpful.
I wrote and improved a bunch of config and m4 scripts that work fine for other projects over the years. The problem is not to do it perfectly, but to deal with autotool versions and OS compatibility issues. For example, libtool is not installed on MacOS, so this needs to be installed/copied from elsewhere (not something ugrep needs though). And this stupid subdir warning crap cannot be fixed on MacOS, because Implementing the suggested changes always breaks the scripts on that OS. I've wasted enough time in the past to try to make the scripts perfect. Now I just make it work to do its business while avoiding pitfalls.
The most recent dumb issue was to discover that bash does not use the executable name for argv[0] when installed as a symlink, but rather uses the target executable name #315. That's why ug is installed as a copy, not as a symlink. Symlink to a local executable works to get the proper argv[0] to return ug, but installing as a symlink on a bin path fails and args[0] returns ugrep (the symlink target). Perhaps a hard link works. But copy it is, for now!
It's easy to criticize, but if it is an easy fix then just do it. Just make sure it works on all *nix OS not just Linux, e.g. Cygwin, MacOS, Solaris, HP-UX etc. and also with all shells e.g. bash, dash, fish, zsh, csh, tcsh.
EDIT: clarify first sentence. "Learn and move on. Don't let ego get in the way of being productive and helpful." which I had meant to convey.
With all respect for lessons learnt through navigation of the political currents of office life, sometimes there are technical reasons why prevailing consensus ought to be set aside in favour of a clean, maintainable, implementation, especially when the merit is proven. Sometimes the authority of bosses is not derived from expertise, but from rank, and a good boss will appreciate a technically superior solution, even at the cost of a bruised ego.
The early releases of autoconf were an M4 spaghetti mess compared to modern (v2.65+). In the 'old days' developers had to figure out from scratch how to detect various features. Many/most inconsistencies have been addressed over time. A modern configure.ac script should be very readable to someone with no m4 experience, and also easy to write without m4 experience. autoconf-archive is a gold mine for abstracting nitty-gritty portability issues away, and also for avoiding the reinvention of the wheel.
I have noticed in the git history (1c131c3, e0aa524, 6be7f9d) how the cp2bin target was initially defective, and it has gone through some unconvincing re-iterations. That was an example of rolling-your-own rather than use long-documented install-hooks, and it remains broken in ways you've not yet discovered. The lesson to be learned there was, to avoid reinventing the installation wheel. Automake's creators have also spent many years specifically understanding and abstracting compatibility issues like this.
My proposed changes will also amount to a complete overhaul of the build automation, but I think you will love it, Dr. van Engelen. I find it difficult to imagine the scripts getting into more trouble than they are now, although I've taken your guarantee under advisement.
Are there any plans to implement a test suite (ie, make check)? I see there are some tests in tests/verify.sh, awkwardly hooked via a non-standard test target in the top directory, but they eschew use of the built-in Parallel Test Harness - perhaps another reinvention that can be avoided. My vscode extension automatically discovers and exposes tests in the Parallel Test Harness, but of course has no hope of handling home-grown test infrastructure. The point of the extension is to expose the typical capabilities of a software project (edit, build, test, debug, package), as long as the project does adhere to autotools documention.
@alerque it seems we are headed for needless duplication of effort - can we coordinate? I have pushed my first (tiny, compared to what I have planned) change to Mergesium:autotools and will create a PR when I'm ready. I'm now looking into lib/Makefile.am which I intend to remove. Would you push what you have done so far so I can build on top of it?
@drok sounds good! @alerque please coordinate efforts, thanks!
Indeed, my past experiences to work with (or around) legacy tools has made me more cautious and a bit of a do-it-yourself kind of guy.
Adding to the list of pitfalls: do not remove SIMD_FLAGS="-march=native -DHAVE_NEON", because NEON builds (ARM6) fail, e.g. RPi zero. Using -march=native is ugly and should not be done, but we need this flag until this is fixed in GCC (good luck). This is the only way to get it to build. I spent over a day searching and trying alternatives back when I wrote this. Something like this is sideways, but necessary at the time.
Re SIMD_FLAGS I assume you are referring to this change:
https://github.com/Genivia/ugrep/compare/master...Mergesium:ugrep:autotools#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77L2-R13
I am removing SIMD_FLAGS from the CPPFLAGS variable because previously/currently, as you've shown, it was used as both a pre-processor variable and a compiler option variable; a mixed-purpose unclean practice. SIMD_FLAGS is used in the portable detection script AX_EXT strictly as a compilation flag as described here; preprocessor defines like HAVE_{extension} will be set in config.h which is used at pre-processing time via the libreflex_a_CPPFLAGS = -include $(CONFIG_HEADER) snippet.
I am separating compiler flags from preprocessor flags to bring the scripts to a standard, boring, canonical alignment with automake flag variables documentation - Casually assigning *FLAGS variables is one root cause for the current mess, so naturally it's one of the main things I'm cleaning up. I apologize if I sound critical, but note I'm working on a solution rather than merely pointing out the problems.
The AX_EXT.m4 script as distributed with autoconf-archive does not detect ARM flags, and I haven't found an equivalent script that supports ARM; I will likely amend the existing AX_EXT.m4 to add ARM detections. It would help if your continuous integration scripts would test building on ARM and other platforms you support, like various CPUs or OS's.
A plausible way you can claim to support other OS's without specifically testing them is if your build system follows the autotools documentation and best practices to the letter or deviates only with the greatest care. Presumably that's why you're using autotools, for the wide OS/compiler compatibility it brings.
I'm no ARM expert, so I hesitate to add support to AX_EXT.m4 - maybe that's something you can do. It would need to detect CPU capabilities, possibly using special purpose feature registers, even though your project doesn't need them. Detecting target features with the -march=native option definitely breaks cross-compilation, which is a popular way to screw up autotools automation, and would not be acceptable in AX_EXT.m4. That flag needs to go, GCC bug or not.
Whether there are GCC bugs related to feature detection should not be your concern as a software developer. It is the purpose of the automake maintainers to work out how to support buggy software, and track when or if that software is fixed and when and if the workarounds should be removed. Normally workarounds should never be removed, so it is possible at any point in the future to build the software on a system where only buggy compilers are available. That's one of the core values of using autoconf in the first place. Don't try to get around it with hacks, it's trying to help you focus on big-brain algorithms, instead of the grunt work of build automation scriptology.
Do you have notes on what the "GCC bug" is, or what issue you were solving and your findings? I am not familiar with the intimate history of this project. I can read the (terse) git commit messages, linked issues, and extrapolate here and there, but I'm not aware of what bug you were trying to work around.
RPi and other SBC use older compiler versions. So even if GCC has addressed this, it won't be something to count on for a while.
Just try to test build on RPi zero with GCC (default) with and without -march=native and -mfpu options. You'll see what I mean. It built fine on RPi3, but not on RPi zero with GCC. I don't remember the exact details from about two/three years ago and it probably was logged with the RE/flex project that ugrep uses. I sure want to build on RPi SBC like the zero. This is the only way I could get it to work to detect NEON and compile for NEON. There are some posts online that share some similarities with the problems I saw, although not 100% the same, like this one for example https://stackoverflow.com/questions/37050532/detect-arm-neon-availability-in-the-preprocessor so there is certainly some funny stuff going on that cannot be cast aside as irrelevant.
Whether there are GCC bugs related to feature detection should not be your concern as a software developer.
When tooling bugs (GCC including) cause failed builds, issues will be opened to fix. Why not fix them up front? I'm not talking about feature detection as the main problem. Failed builds are a problem. Wrong feature detection is of course also bad when builds fail or when binaries target the wrong CPU.
There is a reason why configure.ac is structured the way it is with explicit code to detect CPUs and enable the necessary compile-time flags. I had tried other "accepted" solutions posted online, but those did not work (ergo the NEON problem was the Achilles heel for me). We must use both compile time and runtime AVX2/AVX512 detection for runtime versioning with have_HW_AVX2() and have_HW_AVX512BW() to run the properly compiled code. Messing with these will break ugrep's binary portability on SSE2/AVX2/AVX512 systems (at least SSE2 support is expected when compiled on a SSE2/AVX2/AVX512 capable machine). When built on a non-SSE2 machine, ugrep simply never assumes SSE2, AVX2 or AVX512 is available nor use that code. The only missing part right now in configure.ac is to always compile AVX512 also on an AVX2 machine, so the performance is there when binary ported from an AVX2 machine to an AVX512 machine. Right now, when build on an AVX2 machine, the AVX512 capabilities are disabled (not compiled).
EDIT: fix typo
Thank you, that explains the wonkiness, and lead to the relevant SO answer on How to check the existence of NEON on arm
Back when I wrote the configure.ac code I made sure it detects NEON/AArch64 and doesn't fail detection and doesn't fail to build ugrep and RE/flex. I verified this with RPi3, RPi zero, Android device with termux, and Mac M1. The SSE2/AVX2/AVX512 detection is easy and can be done with another existing solution, but it has to align with the NEON/AArch64 detection and compilation that I verified to work. So I thought it would be better to do this explicitly with AC_COMPILE_IFELSE and AC_LANG_PROGRAM and not rely on another (third-party) m4 solution to detect SSE/AVX and try to adapt that to make it work somehow.
I agree that AC_COMPILE_IFELSE and AC_LANG_PROGRAM and awkward use of -march=native is something you don't typically see in configure.ac scripts. But ugrep and RE/flex aren't your typical projects either.
I also found over time that there aren't any m4 scripts for PCRE2 and some of the compression libraries. So I wrote these and sent the pcre2 one to the GNU org: https://www.gnu.org/software/autoconf-archive/ax_check_pcre2.html
These are other m4s I wrote that are not posted on the GNU org: ax_check_brotlilib.m4 ax_check_bz2lib.m4 ax_check_bzip3lib.m4 ax_check_lz4lib.m4 ax_check_lzmalib.m4 ax_check_zstdlib.m4
I also wrote a new ax_boost_regex.m4 because the one that GNU org posts does not appear to work as documented.
Perhaps other folks may find them useful? If so, let's send them to the GNU org.
The reason pcre2 m4 scripts don't exist is because they are not needed. pcre2 ships pkg-config .pc files, and thousands of projects that depend on pcre2 had no problem with them (see Mergesium@6978c50). Your ax_check_pcre2.m4 file is a disservice to uninformed devs, who may believe it the proper way to integrate pcre2 in their projects, and makes it look like autotools requires all these elaborate hacks to accomplish the most basic of tasks. 👎🏻
As for the other compression detection m4's, I propose in #370 you remove them for the same reason. There is no reason to reinvent the wheel when a proper way is documented in the automake manual.
ax_check_brotlilib.m4 -> see b596048 ax_check_bz2lib.m4 -> see 8a47971 ax_check_bzip3lib.m4 -> see ebe6400 ax_check_lz4lib.m4 -> see a27012a ax_check_lzmalib.m4 -> see 9423c6b ax_check_zstdlib.m4 -> see 4d2b26d
You say "ugrep and RE/flex aren't your typical projects" - I disagree. They are straight-forward projects that don't require any special consideration outside documented autotools features that have existed for many, many years. In the PR #370 I removed all of special scripts, because they added complication and broke cross-compiling, portability and readability. All hooks are removed, except for "install-exec-hook", which is implemented as per automake manual to symlink binaries.
Nice work.
So to verify a few things, I tested the PKG_CHECK_MODULES approach for PCRE2 instead of the m4 file on a MacOS M1 and I've regenerated configure. More specifically, this proposed change:
https://github.com/Genivia/ugrep/pull/370/commits/6978c50c0d60226a076ea1ff78ea4d34ed1be1c4
fails to link pcre2:
Undefined symbols for architecture arm64:
"_pcre2_code_copy_with_tables_8", referenced from:
reflex::PCRE2Matcher::pattern(reflex::PCRE2Matcher const&) in ugrep-ugrep.o
reflex::PCRE2Matcher::PCRE2Matcher(reflex::PCRE2Matcher const&) in ugrep-ugrep.o
...
Part of the problem might be the installers on MacOS such as Homebrew place libs in their specific /opt/homebrew/ locations that are tested in my m4. Where, besides the PKG_CHECK_MODULES in configure.ac is this change applicable to link against pcre2?
A second thing I tried is something that annoyed me relentlessly about autotools, because of the subdir warnings. So I was hoping for a miracle. But the _INIT_AUTOMAKE([foreign subdir-objects]) won't work either, as I suspected, because this never reliably worked on MacOS for other projects. Indeed, trying https://github.com/Genivia/ugrep/pull/370/commits/83b91d1b1d315df46824dc6478fc3e32899c1dfc gives:
$ aclocal
$ autoheader
$ automake --add-missing --foreign
$ autoconf
$ automake
configure.ac:3: error: AM_INIT_AUTOMAKE expanded multiple times
/opt/local/share/aclocal-1.16/init.m4:29: AM_INIT_AUTOMAKE is expanded from...
configure.ac:2: the top level
/opt/local/share/aclocal-1.16/init.m4:29: AM_INIT_AUTOMAKE is expanded from...
configure.ac:3: the top level
autom4te: error: /opt/local/bin/gm4 failed with exit status: 1
aclocal: error: autom4te failed with exit status: 1
EDIT: aha, I made the changes and AM_INIT_AUTOMAKE works. I was skeptical, because a couple of years back the same AM_INIT_AUTOMAKE did not work on MacOS for another project and I am sure we made the exact recommended changes, probably because a few year back we still were forced to use older autotools. This looks better now.
As for the other compression detection m4's, I propose in #370 you remove them for the same reason. There is no reason to reinvent the wheel when a proper way is documented in the automake manual.
ax_check_brotlilib.m4 -> see b596048 ax_check_bz2lib.m4 -> see 8a47971 ax_check_bzip3lib.m4 -> see ebe6400 ax_check_lz4lib.m4 -> see a27012a ax_check_lzmalib.m4 -> see 9423c6b ax_check_zstdlib.m4 -> see 4d2b26d
You say "ugrep and RE/flex aren't your typical projects" - I disagree. They are straight-forward projects that don't require any special consideration outside documented autotools features that have existed for many, many years. In the PR #370 I removed all of special scripts, because they added complication and broke cross-compiling, portability and readability. All hooks are removed, except for "install-exec-hook", which is implemented as per automake manual to symlink binaries.
Sigh. These suggested changes do not work on MacOS, probably for the same reason PCRE2 is detected but does not link. I will also try a Mac Intel Monterey which are still frequently used, but that's likely to throw the same errors. When I said that this project that isn't typical, it means that I've tested portability to many platforms, not just Linux or BSD Unix that have autotools installed and made sure SIMD is detected and compiled, which wasn't working for ARM6 with the usual stuff back then.
After adding PKG_CHECK_MODULES([ZSTD], [libzstd], to replace AX_CHECK_ZSTDLIB:
$ aclocal
$ autoheader
$ automake --add-missing --foreign
$ autoconf
$ automake
$ ./build.sh
...
checking for libzstd... yes
...
g++ -std=gnu++11 -Wall -Wextra -Wunused -O2 -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/local/lib -L/opt/local/lib -L/opt/local/lib -L/opt/local/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L../lzma/C -o ugrep ugrep-ugrep.o ugrep-cnf.o ugrep-glob.o ugrep-output.o ugrep-query.o ugrep-screen.o ugrep-stats.o ugrep-vkey.o ugrep-zopen.o -lpthread ../lib/libreflex.a -lviiz -lbrotlidec -lbrotlienc -llz4 -llzma -lbz2 -lz -lpcre2-8
...
Undefined symbols for architecture arm64:
"_ZSTD_DStreamInSize", referenced from:
zstreambuf::open(char const*, __sFILE*) in ugrep-ugrep.o
zstreambuf::next(unsigned char*, unsigned long) in ugrep-ugrep.o
...
Part of the problem might be the installers on MacOS such as Homebrew place libs in their specific
/opt/homebrew/locations that are tested in my m4. Where, besides thePKG_CHECK_MODULESin configure.ac is this change applicable to link against pcre2?
The PCRE2_LIBS and PCRE_CFLAGS variables calculated by PKG_CHECK_MODULES are used in src/Makefile.am in the ugrep_LDADD and ugrep_CXXFLAGS variables respectively: https://github.com/Genivia/ugrep/commit/6978c50c0d60226a076ea1ff78ea4d34ed1be1c4#diff-4cb884d03ebb901069e4ee5de5d02538c40dd9b39919c615d8eaa9d364bbbd77
I also noted in the commit message for 6978c50 that it is WIP. Specifically I got undefined references when compiling with -O2 but not with -O0 - something is amiss, but I don't know what, yet. It's possible that your undefined reference to _pcre2_code_copy_with_tables_8 is a clue.
-O2
Would you please try with -O0 to confirm whether you are also seeing a sensitivity to optimization? I have a feeling that the problem is not with PKG_CHECK_MODULES at all, but some other missed flag, or possibly the order the libs are enumerated on the link cmd line.
In any case, a hardcoded path list /usr/local /usr /opt/homebrew /opt/local /sw may very well work for you, but it breaks cross compilation, and it won't work for distros that have another filesystem layout convention. To be portable, you cannot rely on hardcoded paths at all, even those that you tested and work for you.
When cross-compiling, for instance, pkg-config searches a list of paths that include sysroots but does not include the system's running paths. In that case, all the paths you list are wrong places to look for the libraries; those places hold libs for the wrong architecture, not the target architecture on the --host= configuration option.
SIMD is detected and compiled, which wasn't working for ARM6
ARMv6 does not have hardware FPU and thus would never support SIMD extensions if I understand correctly. Is there a reason to expect SIMD to be supported when targeting ARMv6 ?
ARMv7 has optional NEON support (it's up to the CPU manufacturer) ARMv8 NEON support is mandatory, and there is no -mfpu=neon flag because it's always on.
I have no hands-on experience with ARM's; this is my understanding from reading documentation.
SIMD is detected and compiled, which wasn't working for ARM6
ARMv6 does not have hardware FPU and thus would never support SIMD extensions if I understand correctly. Is there a reason to expect SIMD to be supported when targeting ARMv6 ?
ARMv7 has optional NEON support (it's up to the CPU manufacturer) ARMv8 NEON support is mandatory, and there is no -mfpu=neon flag because it's always on.
I have no hands-on experience with ARM's; this is my understanding from reading documentation.
I may have confused ARM6 with ARM7 in my wording. In this case I didn't keep notes with details, because I got it working fine to detect NEON properly. I'm running an RPi zero again this morning to check, and to set it up for testing the suggested changes. The problem was either that NEON was not detected or detected incorrectly. Incorrect detection ran into a crash, obviously. This is what I remember battling with a few years back. Giving up NEON was not an option for me.
Got it with the PCRE2_LIBS part and the other library dependencies, which test OK now. However, I find this required change to the Makefile.am ugly and suspect it increases maintenance fragility. Why not accumulate the library dependencies in LIBS and LDFLAGS in configure? That approach will never lead to a problem when a Makefile.am omits a library-dependent XYZ_LIBS, for example when a new optional dependency is introduced and thus more scripts need to be changed. Perhaps I'm ignoring best practices by accumulating library dependences in LIBS, but not doing so looks wrong to me as more fragile. And yes, for cross-platform library paths it is not great to have the paths in the ax check m4 scripts in the way folks used to rely on in the past for decades.
The completions compgen scripts I wrote to generate fish and zsh completion files save me time from hand-coding the completion files when options change. But the scripts only need to be run when options have changed. The man.sh script must be run for each release to update the auto-generated man page version stamp and contents, mostly from the ugrep --help output of options modified with sed. I still run man.sh by hand, because I have to copy the generated man.txt in the README. All these scripts use sed extensively. Sed has differences across platforms, so that can be tricky to run universally if not cross-platform tested carefully beforehand. So I do not auto-generate the man page with a makefile from a template to avoid potential sed or other tool failures. Using sed is great, as long as it works for the more complex regex I have to use.
Why #!/bin/bash instead of #!/bin/sh? If the script is generic, non-bash, then it shouldn't require bash. If a script is bash-specific, then #!/bin/dash is a better choice for efficiency. Either way, if bash is not installed the script won't run so I prefer #!/bin/sh and keep things simple.
The extensive use of @ variables such as @PACKAGE_NAME@ in Makefile.am is overdoing it IMHO. The project name won't change and using these variables makes scripts less readable to those who aren't skilled in autotools. I have the same concern with PACKAGE_VERSION. The version string should be kept literally in ugrep.hpp. Remember that the project should also be compiled without autotools, such as with MSVC++. Sure, MSVC++ nowadays supports makefiles etc. but doing all that adds more baggage than it is worth. For RE/flex I used sed in a makemake.sh script to substitute the version number into the source code, which means the source compiles on MSVC++ without worry.
The --disable-simd makes sense, to update from --disable-sse2. I used to have only SSE2/AVX2 support in ugrep and RE/flex, not NEON/AArch64, hence this option. Why did I not change it back then? Because there might be scripts out there that use --disable-sse2 and those scripts will now fail, unless this option can also be used as an "legacy" alternative to --disable-simd.
Other suggested changes look in principle alright to me. But I need to spend more time reading the changes to check them and to run tests.
To follow up on ARM6: RPi zero build will not work with the current configure script. It detects NEON when it's not supported. I don't remember the details when I worked on this, but I may have given up on ARM6 non-NEON detection in favor of making sure NEON is detected on other ARM that supports it.
If we can make detection work properly on all ARM with and without NEON, then that is a win 👍
Why
#!/bin/bashinstead of#!/bin/sh? If the script is generic, non-bash, then it shouldn't require bash. If a script is bash-specific, then#!/bin/dashis a better choice for efficiency. Either way, if bash is not installed the script won't run so I prefer#!/bin/shand keep things simple.
You are referring to ee4bf15 - the commit comment explains the reasoning for the change. I will quote the relevant paragraph below for your convenience:
The compgen file uses bash syntax ($'...') so I changed the shebang to
require bash instead of sh.
To follow up on ARM6: RPi zero build will not work with the current configure script. It detects NEON when it's not supported.
If the compiler installed on your device is capable of generating NEON code, then NEON code should be generated. The presence of NEON features should be done at runtime, and the NEON code paths used only the the feature is available, at runtime. That is the point of b4ba7e5 - it's very possible I implemented the gating or detection wrong - please review.
So either the HWCAP_NEON detection is wrong, or the feature gating on HW.neon is wrong. You'll have to review b4ba7e5 in detail - I don't have arm hardware.
I still run man.sh by hand,
I had not even noticed your man.sh script, since you also keep the generated file in git.
Having a look at it, it's not clear to me that this script does anything useful that autotools doesn't support out-of-the-box. Generating files from templates is literally what autoconf's main purpose is. Why reinvent it, poorly?
You have a hand-written manpage file wrapped in a shell script that prepends a header with the current date and an arbitrary version string given as argument on the command line. Autoconf has logic built in, and accessible as (0767373):
# Allow the manpage be accessed as either ug or ugrep
AC_CONFIG_FILES([man/ugrep.1])
AC_CONFIG_LINKS([man/ug.1:man/ugrep.1])
The extensive use of @ variables such as
@PACKAGE_NAME@in Makefile.am is overdoing it IMHO. The project name won't change
@PACKAGE_NAME@ and @PACKAGE_VERSION@ are placeholders in the template file man/ugrep.1.in that the configure script replaces with the correct values at configuration time.
You may well not change the project name yourself, but being open source, someone may fork and build it for their purposes with a different name, and with customizations of their choosing. Of course they would call it something else, as to not confuse themselves (eg, mygrep is not the same as ugrep).
Some user may well decide to replace GNU grep with your program, on one or all their systems, or maybe even use it as the default grep in their distro, so that it is used in all scripts and every place where GNU grep was used previously. Ie, they make your program a drop-in replacement for GNU grep. They would naturally want it installed in /bin/grep and mark it in their package repo as an alternative to GNU grep. The command 'man grep' should then bring up your manpage, and refer to the program as 'grep' and not 'ugrep'.
I have kept the name 'ugrep' in the copyright notice and issues link at the bottom of the manpage, as there it should really refer to your project ("ugrep", immutable), even if the program name is "grep" (or @PACKAGE_NAME@)
I would not worry about the computer literacy of users that might come in contact with your source code. If they don't know that the "@word@" notation denotes a variable name, they won't know how to run configure or make, and they won't be editing anything. They would more likely be confused by the funny slashes and dots and junk that are manpage syntax. What's the real concern here? You're not worried about confusing them with manpage syntax, but worry about using a variable name instead of hardcoded name for the program name? Your reasoning doesn't add up.
The
--disable-simdmakes sense, to update from--disable-sse2. Why did I not change it back then? Because there might be scripts out there that use--disable-sse2and those scripts will now fail
IMHO it is appropriate to make those scripts fail. That failure is how to signal to the maintainers of those scripts that the configuration options have changed and they need to review the changelog/release notes.
I think you may have missed that I also replaced the --with-zlib package configuration option with --enable-zlib and --disable-zlib which is how you normally enable/disable features of a program. --with is used to request an alternate library be used such as --with-zlib=/home/blah/my-special-zlib (as opposed to the default zlib package installed on the system). The same --enable/--disable convention is now used for all compressions.
distros that package ugrep will have to review configure --help and fix up their package configurations.
Did you know that debian builds with --disable-avx on x86 and --disable-neon on arm? It seems related to https://bugs.debian.org/964957 and appears that the package maintainers at debian have to jump through some hoops to distribute ugrep
I think you should document, when you release an overhauled version, that simd detection is done at runtime, and no longer does SIMD need to be disabled in order to make the binary portable to the target architectures (ie, add the standard INSTALL file to explain unusual expectations). Of course, NEON detection should be fixed to work correctly on all CPUs
So, if I understand this correctly, there is no clear advantage to using @ project variables in scripts for project maintenance, besides creating an assumed dependence on autotools when using these as macros in source code, right?
Overdoing it only adds to fragility, i.e. if something goes wrong or is no longer available, then the problem is no longer isolated. When we don't use autotools for example, or when autotools is replaced with something else or autotools v10.0 comes out and we can't build legacy projects with it any longer. Similar things happened decades ago with projects that required specialized software to run to create and maintain it, when this specialized software is no longer available, and worse, the binaries run on obsolete CPUs.
This is my short list viz. battle plan:
- refactor makemake.sh to run to auto-update the source code tree, man page, completions and configure.ac when bumping a version for release
- only use makefiles when appropriate, i.e. to track source code dependencies to compile/link code, no makefile abuse as if these are shell scripts
- ergo, keep man/ugrep.1 updating and completions updating out of makefiles, use scripts, no makefile abuse
- ergo, keep the ugrep/patterns/ out of makefiles, these are static data to be installed in datadir with a hook; creating makefiles adds fragility to maintenance
- stay clean of required dependence on autotools and/or cmake, i.e. source code must be compilable and linkable without autotools or cmake, recall that autotools/cmake are just tools/hacks to make compiling and linking independent of a platform, it was never meant to be a monopolistic (take it or leave it) system for compilation and linkage, which can always be done "by hand" or with a makefile or with a custom script or with MSVC++ project properties
- fix detection and linkage of libraries such as https://github.com/Genivia/ugrep/pull/370/commits/2fd7710e88caf164acb99f4ed119236bae87b74c
- fix SIMD detection https://github.com/Genivia/ugrep/pull/370/commits/201c50840ab4d65cfbd36d2587748cccc1cce2c6 in particular NEON https://github.com/Genivia/ugrep/pull/370/commits/b4ba7e5ec0cace0c40bc7437f447ff56a0ec6366
Point 7 is important to address for both accurate compile-time and runtime use of SSE2/AVX2/AVX512BW and compile-time NEON/AArch64 use (no runtime check).
After adding
PKG_CHECK_MODULES([ZSTD], [libzstd],to replaceAX_CHECK_ZSTDLIB:
$ ./build.sh
...
checking for libzstd... yes
...
g++ -std=gnu++11 -Wall -Wextra -Wunused -O2 -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/local/lib -L/opt/local/lib -L/opt/local/lib -L/opt/local/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L/opt/homebrew/lib -L../lzma/C -o ugrep ugrep-ugrep.o ugrep-cnf.o ugrep-glob.o ugrep-output.o ugrep-query.o ugrep-screen.o ugrep-stats.o ugrep-vkey.o ugrep-zopen.o -lpthread ../lib/libreflex.a -lviiz -lbrotlidec -lbrotlienc -llz4 -llzma -lbz2 -lz -lpcre2-8
...
Undefined symbols for architecture arm64:
"_ZSTD_DStreamInSize", referenced from:
zstreambuf::open(char const*, __sFILE*) in ugrep-ugrep.o
zstreambuf::next(unsigned char*, unsigned long) in ugrep-ugrep.o
...
Whatever gymnastics the build.sh script does, it is wrong. The linker command you showed does not include -lzstd, which is the reason for the "Undefined symbol _ZSTD_DStreamInSize".
The commit that introduces PKG_CHECK_MODULES([ZSTD], [libzstd] (4d2b26d) adds $(ZSTD_LIBS) to the program's LDADD flags, which you might have omitted.
I propose deleting build.sh - the correct way to build, which correctly handles dependencies is simply 'make' - nothing more is needed.