dbry / WavPack

WavPack encode/decode library, command-line programs, and several plugins
BSD 3-Clause "New" or "Revised" License
371 stars 67 forks source link

Autotools update #101

Closed SoapGentoo closed 3 years ago

SoapGentoo commented 3 years ago

Hi @dbry Here are a few updates to make the Autotools build system POSIX compatible so it can be used with shell like Dash and Make implementations such as bmake and pmake. Furthermore, I've implemented the test to use the normal automake test harness, which means it can be invoked with make check instead of invoking the binary directly. This massively simplifies the packaging for us downstream in the distributions.

dbry commented 3 years ago

Thanks so much for this update! I have to admit that the autotools stuff is a mystery to me and I'm always hesitant to make changes for fear of breaking stuff, so it's nice to have someone in there who knows what they're doing.

I noticed that this broke the Travis CI because the old --enable-tests no longer builds the wvtest binary and I can't have Travis do the --default test because it takes over an hour (I use --exhaustive --short --no-extras on Travis). I tried make check TESTS= locally and it seems to work. I've pushed this and if it passes I'll merge (and fix the README later).

Thanks again!

SoapGentoo commented 3 years ago

@dbry thanks, here's an idea: why don't we change the default test to run your internal check? i.e. run --exhaustive --short --no-extras?

dbry commented 3 years ago

@SoapGentoo Yeah, that might be a good idea too. The tests for the extra modes should be run once in a while, but they are very time consuming.

SoapGentoo commented 3 years ago

@dbry hold off on merging till I've pushed this fix...

dbry commented 3 years ago

And just to be clear, you're going to change the test-wrapper.sh, not the wvtest.c source file, right?

SoapGentoo commented 3 years ago

correct, this is just playing with Automake

SoapGentoo commented 3 years ago

@dbry finished now. Travis should run make distcheck instead, which tests that the final tarball can be built correctly, but also invokes make check to run the test suite.

dbry commented 3 years ago

Thanks!

dbry commented 3 years ago

I am happy with anything you suggest and are willing to implement. As long as it continues to work in the places I use it, and is more up to date, I'm happy.

While I've got your attention, I do have a couple questions. When I run ./configure on my system I get this suggestion from libtoolize:

libtoolize: Consider adding 'AC_CONFIG_MACRO_DIRS([m4])' to configure.ac,
libtoolize: and rerunning libtoolize and aclocal.
libtoolize: Consider adding '-I m4' to ACLOCAL_AMFLAGS in Makefile.am.

Is this anything I should be concerned about or act on?

Also, there was a discussion in issue #93 that ended up being my having outdated config.guess and config.sub files. The files included in my tarball were the latest offered in my distribution, but those were not the very latest, and the suggestion was that I should just grab the latest two copies of those files off the web when I create a tarball. Do you agree that that's the right way to go for my next release? I just looked and my copies of those files are still over 3 years old.

Thanks again!

SoapGentoo commented 3 years ago
  1. Yes, all changes generally won't change the workflow, just make it faster, more portable and easier for us to handle downstream (this pertains to all distros, not just Gentoo)
  2. The message you've posted is harmless. Historically, the directive to tell aclocal where all your macros were was done through ACLOCAL_AMFLAGS in Makefile.am. This is deprecated, won't be supported with Automake 2.0 (when and if that even ever hits). Currently, just ignore it.
  3. Keeping config.guess and config.sub up to date is tricky, given how slow the Autoconf release cycle is. Most distros (Gentoo included) have hooks to replace those two files just before running ./configure, so we generally don't see these issues. You should keep them updated, which is tricky when whatever provided them to you doesn't update them. Consider replacing them locally. config.guess and config.sub are one of those problematic parts of Autoconf, as they define system-relevant tuples that do get outdated over time and make going back to historic releases much harder. In Gentoo we have an extra package to keep them up to date, but this likely won't translate to whatever platform you're using.
dbry commented 3 years ago

Thanks, sounds good.

Given the third item, it might make sense to spin up a Gentoo VM when I create a tarball. Fortunately I do only about one new release a year, so it would not be onerous. Or I could just get you to do it... :smiley:

SoapGentoo commented 3 years ago

sure, just ping me and I can cut a release too

dbry commented 2 years ago

@SoapGentoo It's been over a year, but I'm finally ready for a new release!

I am able to build a tarball distro and it works fine, but the config.guess and config.sub are from 2018, which I remember was a problem before. I installed Debian 11.3 in a VM, but it had the same old versions! I could just grab the latest from the web and use them, but I'm not sure that's the "correct" way to do it. I installed dh-autoreconf but the instructions really don't make sense to me (because I really have no idea what I'm doing).

So I'm going to take you up on your offer of cutting a release for me as everything is ready to go for 5.5.0. It probably wouldn't hurt to look it over to make sure I didn't do anything stupid. Specifically, I did make a couple simple fixes to Makefile.am to include the documentation in the tarball and put the generated man pages in the correct folder, plus the earlier change to include a Windows resource file.

@evpobr Any other comments before I pull the trigger here are also appreciated!

Thanks in advance!

evpobr commented 2 years ago

Hi. I'm a little late, but I don't see any obvious problems.

SoapGentoo commented 2 years ago

@dbry when bootstrapping the tarball, I found a minor (no risk) issues with the Autoconf. I'll send in a quick PR and then I produce the tarball afterwards?

dbry commented 2 years ago

@SoapGentoo Thanks, I have merged your PR. I also made another fix for a bug just reported. It's funny, because right before releases suddenly people find bugs (from fuzzing, obviously). But I guess that's better than right after the release!

In any event, I think I'm ready for the tarball, but I reserve the right to ask for another if another bug comes in before I regenerate all the Windows binaries... :)

SoapGentoo commented 2 years ago

https://dev.gentoo.org/~soap/distfiles/wavpack-5.5.0.tar.xz

I had to upload it to my Gentoo webspace, since Github won't allow me to attach tarballs. Here's a few timestamps: File internal timestamp
config.guess timestamp='2022-05-08'
config.sub timestamp='2022-01-03'

I hope that's fresh enough for you :wink: Let me know if you need a fresh bootstrap.

dbry commented 2 years ago

Thanks so much for this! Just to be sure the MD5 is 8d5fe248d4fa327a0915a4d6705b4a52, right?

And yes, those were the latest dates I found for config.[guess|sub] too. I'll be curious to see if any other files are different from what I would have generated here.

After a little thought I realized that if a source file did change, I could just swap that out, right? I assume there's no other integrity mechanism preventing that. Of course, I'm only talking about a couple days at the most.

Cheers!

SoapGentoo commented 2 years ago

The MD5 is correct.

In theory you could replace files, but there might be coupled macros between ./configure and say the libtool script, so before replacing any files, I'd check whether it's really necessary. You can diff the files to any other autoconf-2.71 generated configure and convince yourself that everything is sane (but not necessarily identical, given that the Gentoo autoconf might be slightly different/patched).

dbry commented 2 years ago

Well, I found an obscure issue with big-endian machines (testing on QEMU/Debian) and decided to fix it before the release.

Only 3 files changed: src/pack_utils.c and ChangeLog and NEWS. I was able to update those in-place and everything seems fine. However, it's a little odd because the dates on the those files is different than all the others. I wouldn't think that matters and I'll go with it unless you feel like making another tarball. Either way I think is fine.

Thanks!

SoapGentoo commented 2 years ago

Uploaded a fresh tarball for you: 9501de7a8ac23649b06f4e470d5ff299 MD5, same file name (the old file is wavpack-5.5.0-old.tar.xz now)

dbry commented 2 years ago

I verified that all files matched mine, but the dates are better. Will release this evening.

Thanks again for all your help!

SoapGentoo commented 2 years ago

@dbry just added 5.5.0 to the tree, and spotted this:

/var/tmp/portage/media-sound/wavpack-5.5.0/work/wavpack-5.5.0/cli/wvtest.c: In function ‘run_test’:
/var/tmp/portage/media-sound/wavpack-5.5.0/work/wavpack-5.5.0/cli/wvtest.c:963:62: warning: cast from pointer to integer of different size [-Wpointer-to-int-cast]
  963 |             printf ("decode_thread() returned error %lld\n", (long long) term_value);
      |                                                              ^

wouldn't it make more sense to use uintptr_t here (and the equivalent C99 format specifier)?

EDIT: I'm seeing this because we're compiling for 32-bit, and long long is 64-bit even on x86, whereas uintptr_t is always the native type.

dbry commented 2 years ago

@SoapGentoo Yeah, this has been bugging me a little, generating warnings on some setups. I settled on what I have now because, despite warnings, it will (1) always display the correct value and (2) will work even on old (pre C99) compilers. I got burned recently when someone helpfully replaced some printf() conversions to %zu for printing size_t variables and then I discovered that MSVC didn't have that until 2013 (which means you get nothing printed) and a lot of people were building that project on ancient setups (retro gaming and such). That was much smaller and whether this project gets compiled on many pre-C99 compilers is an open question, but I was using MSVC 2008 until a year ago!

But more generally, it's actually silly to print the value at all because that thread doesn't set any error result. I think I'll just eliminate printing the value altogether. Thanks for reminding me.

dbry commented 2 years ago

@SoapGentoo So sorry to bother you again, but I'm preparing for a new release and was hoping you could make me a new tarball with the latest config files. Thanks in advance! :smile:

SoapGentoo commented 2 years ago

sure, as always, let me know when you need it exactly. Is the code ready now?

dbry commented 2 years ago

@SoapGentoo, just pushed the last ChangeLog update, so any time.

Thanks!

SoapGentoo commented 2 years ago

https://dev.gentoo.org/~soap/distfiles/wavpack-5.6.0.tar.xz SHA256: af8035f457509c3d338b895875228a9b81de276c88c79bb2d3e31d9b605da9a9 wavpack-5.6.0.tar.xz

dbry commented 2 years ago

Thanks, SHA verified! :smile_cat: