i don't understand "the fix in gnulib modifies m4"

do you mean "the fix in gnulib is required in m4"?

there has not been any update to the release branch of m4 since Jan 2023, when they updated gnulib checkout and some other small things, I'm assuming you will need to add a similar patch like this in order to bump it again

https://git.savannah.gnu.org/cgit/m4.git/commit/?h=branch-1.4&id=3943e7355e4749ea53a8a5319a43926723c56a39

I actually don't understand how that works though, or what an "m" filetype is, but it shouldn't be difficult right?

first commit to do so updated docs too

HACKING: Mention how to use submodule.

https://git.savannah.gnu.org/cgit/m4.git/commit?h=branch-1.4&id=6942f58cb5bb624a28672658a4369b85bce77783

perhaps we can have gnulib as a separate tool?

If you already have
  another gnulib clone on your disk, you can use the environment
  variable GNULIB_SRCDIR to point to the previous checkout to speed up
  the process.

i don't understand "the fix in gnulib modifies m4"

do you mean "the fix in gnulib is required in m4"?

there has not been any update to the release branch of m4 since Jan 2023, when they updated gnulib checkout and some other small things, I'm assuming you will need to add a similar patch like this in order to bump it again

https://git.savannah.gnu.org/cgit/m4.git/commit/?h=branch-1.4&id=3943e7355e4749ea53a8a5319a43926723c56a39

@mpratt14

git clone https://git.savannah.gnu.org/git/gnulib.git
git show 7352d9fec59398c76c3bb8a2ef86ba58818f0342

This gnulib commit is related to the macOS issue with corrupted sparse file copies and modifies m4/lseek.m4. From what I just discussed with the coreutils team
https://debbugs.gnu.org/cgi/bugreport.cgi?bug=61386#268

I would assume that tools/m4 is only used to generate the staging_dir/host/bin/m4 executable, which is used during the build, so no changes are needed there. The coreutils-9.2 tarball we download already has an m4 directory with the fix applied. I had the wrong impression that sources from the legacy version from tools/m4 are used in the coreutils build. So I opened the discussion to make sure we are on the safe side.

We should also decide what to do with this patch: https://github.com/openwrt/openwrt/blob/master/tools/coreutils/patches/001-m4.patch

Currently the build requires it, and if we remove it, it fails with the following error:
https://httpstorm.com/share/.openwrt/test/2023-02-06_coreutils-9.1/2023-03-21-openwrt-build-coreutils-9.2-no-patch.txt

However outside the OpenWRT build environment, coreutils can be compiled successfully from the tarball:

wget https://ftp.gnu.org/pub/gnu/coreutils/coreutils-9.2.tar.xz
xz -dd coreutils-9.2.tar.xz
tar xvf coreutils-9.2.tar
cd coreutils-9.2
./configure
make -j 16

This looks like incorrectly configured include path or alike. If we can fix that and then the patch will not longer be needed. What do you think?

@mpratt14

I actually don't understand how that works though, or what an "m" filetype is, but it shouldn't be difficult right?
perhaps we can have gnulib as a separate tool?

I have no experience with the m4 macro processor.
Since the coreutils tarball comes packed with its own m4 sources, we shouldn't have to worry about changes to tools/m4 or adding gnulib.

in that case we just have to refer to old conversations

#9347 (comment)

like I said, if gnulib happens to be providing macros that are in missing-macros, IMO we should add gnulib as a separate tool and link coreutils and others to it and get rid of these patches, but I didn't take a close look yet so I don't know for sure if that's the best option

@mpratt14 Good find, Michael!
The comment from your link talks about PKG_FIXUP:=autoreconf. If I remove that line from tools/coreutils/Makefile, the build is successful. The tarball we download is ready to ./configure && make as it is. My observations show that with PKG_FIXUP:=autoreconf, the configure script is modified to a broken state, where the GNULIBHEADERS_OVERRIDE_WINT_T variable ends up with an empty value in Makefile. Notably a block from configure lines 21914-21952 is removed and it is responsible for setting the variable. The empty value is filled in a header file ./lib/stdint.h resulting in corrupted #if statement, which breaks the build.

Normally when coreutils is reconfigured, files from the m4 sub-directory are used. However the OpenWRT build uses old versions of these files from staging_dir/host/share/aclocal. If I overwrite the old files there from coreutils/m4, coretutils configures and builds correctly. So an update to aclocal might also help, though from what I read, it might also break other stuff. The priority in which m4 files are used is another issue that should be looked into. Files from the project should have preference.

At this point I believe we should do the following:

1. update to coreutils-9.2
2. remove PKG_FIXUP:=autoreconf from Makefile
3. remove patches/001-m4.patch

Note that as long as we don't reconfigure, the patch is not needed. If we add the patch, it will force reconfigure the project, since m4 files are changed. This works. but may not be optimal, because the build should use files from coreutils/m4, but OpenWRT uses legacy files from staging_dir/host/share/aclocal.

like I said, if gnulib happens to be providing macros that are in missing-macros, IMO we should add gnulib as a separate tool and link coreutils and others to it and get rid of these patches, but I didn't take a close look yet so I don't know for sure if that's the best option

Seems like a good idea, considering gnulib includes the latest version of m4. Can you do it?

@Ansuel @neheb @aparcar @zhanhb
What do you think?

I'm putting together a PR to use gnulib as a separate build tool (just like missing-macros to be honest), which would be one of several methods for resolving the difference between macros in these gnu programs where some are older than others, but we need recent macro fixes

that way, both m4 and coreutils can be bootstrapped using the same checkout of gnulib and have all the same macros at build time and in aclocal at the same time

@mpratt14 @Ansuel @hauke @aparcar
Hosts tested: Ubuntu Linux 22.04, macOS 12.6.3
Targets tested: WRT3200ACM, x64, TL-WR1043ND v4

so it is no longer having a conflict with missing-macros? what changed?

@mpratt14

so it is no longer having a conflict with missing-macros? what changed?

PKG_FIXUP:=autoreconf in the Makefile causes the configure script to get recreated. Normally that should use the m4 directory from coreutils and succeed. But OpenWRT improperly uses legacy m4 files from staging_dir/host/share/aclocal which produce a faulty configure script, that in turn produces a faulty Makefile lacking a GNULIBHEADERS_OVERRIDE_WINT_T value, that in turn produces a corrupted #if statement in ./lib/stdint.h and the build fails.
As long as we don't regenerate the configure script, it will build correctly, so I removed PKG_FIXUP:=autoreconf and now it works. The only limitation is that if we try to patch any of the m4 files at a later point, I expect it to recreate the configure script and break.
Can you configure the build system to use m4 from the project when available? That should make for a permanent fix to any project using m4. Or perhaps add gnulib as you suggested and use latest m4 files from there, but then some projects may require older versions and break.

the autoreconf step and the patch was added at the same time, so I would expect the conflict to happen again when removing both the autoreconf step and patch without something else changing since then

did that patch apply cleanly to 9.2, or did they make changes to that patched macro function as well? maybe they are coincidentally the same now

some projects may require older versions and break.

I came across build issues with the patch tool which can bootstrap the same way as others, but since it is a 5 years old release it cannot work with latest gnulib

the rest have minor problems that can be easily solved manually

@mpratt14
The coreutils-9.2 release makes many changes to m4 files. The old patches/001-m4.patch still applies and builds successfully. It does require a refresh, but there are no conflicts. I see three options:

1. remove the patch and remove PKG_FIXUP:=autoreconf: this results in properly configured coreutils based on the m4 files that come with it (original configure script that came in the tarball). We can patch any source files without triggering reconfiguration. Builds and runs fine. A working OpenWRT firmware is created. But if we change any m4 file, the build is likely to fail because legacy external m4 files will be used. This needs to be addressed by the build system.
2. keep the patch and keep PKG_FIXUP:=autoreconf: the configure script is always regenerated based on legacy external m4 files and differs from the original: many blocks are missing, changed or rearranged. It is not clear how correct the configuration is, but it compiles and builds a firmware image. I have not tried deploying the firmware, but I expect it to work. We can patch m4 files without breaking the build.
3. keep the patch and remove PKG_FIXUP:=autoreconf: same as 2.

https://httpstorm.com/share/.openwrt/test/2023-02-06_coreutils-9.1/2023-03-23-openwrt-build-coreutils-9.2-no-patch.txt
https://httpstorm.com/share/.openwrt/test/2023-02-06_coreutils-9.1/2023-03-23-openwrt-build-coreutils-9.2-patch.txt

Can you change the build system so that if an m4 directory exists in the project, it is preferred?

I recommend we grab this patch, but forget the changes to NEWS since it will conflict

https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=093a8b4bfaba60005f14493ce7ef11ed665a0176

might as well get this one too since date is one of the things we install

https://git.savannah.gnu.org/cgit/coreutils.git/commit/?id=9c5e542fd190a14431092e3b6cb45d18fe95f26f

@mpratt14 Hi Michael!
Would you like me to add these patches to the current PR or you can also push them after we update to 9.2?
This PR is ready to merge as it is. Did you find how to add gnulib to the build system?

I would do it in the same commit

I'm ready to put my PR for gnulib, but it requires rebase and I want to put it on top of this directly

i mean the same commit as this update to 9.2

@mpratt14
I added the upstream patches you requested to the PR. Edit: test build successful.
Last week I also made a PR to update the coreutils package openwrt/packages#20711
Let me know if you want any changes there.

add the commit messages and headers with commit, date, subject, so later everyone can see that these are backports

when you're done run

make tools/coreutils/{clean,refresh} V=sc

(which removes the "git diff" and "index" lines I think, otherwise you can do it manually since its not needed)

@mpratt14 Thanks! Is it good now?

now I'm going to be very picky and specific...

in the commit message put the links at the bottom with either footnote numbers [1] or commit tag Link:

I would link directly to the GNU bug report instead of the other pull request

you don't have to mention me, but if you do write mcpratt (I plan to change username at some point haha)
or do Co-developed-by: tag at the bottom but before your signoff

I also test built on my system, everything else looks good to me 👍🏼

@mpratt14 Ready!

@hauke Thank you! Can you please also review the coreutils package PR? openwrt/packages#20711