Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autotools update #101

Merged
merged 4 commits into from
Apr 26, 2021
Merged

Autotools update #101

merged 4 commits into from
Apr 26, 2021

Conversation

SoapGentoo
Copy link
Contributor

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
Copy link
Owner

dbry commented Apr 25, 2021

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
Copy link
Contributor Author

@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
Copy link
Owner

dbry commented Apr 25, 2021

@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
Copy link
Contributor Author

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

@dbry
Copy link
Owner

dbry commented Apr 25, 2021

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

@SoapGentoo
Copy link
Contributor Author

correct, this is just playing with Automake

* `==` is not compatible with stricter POSIX shells
  such as the dash shell.

Bug: https://bugs.gentoo.org/773955
* These work with `bmake`/`pmake`
* Globs don't work in Automake
* To run the full test suite:
  make check TESTS=all-tests
wvtest_SOURCES = wvtest.c md5.c
wvtest_CFLAGS = $(AM_CFLAGS) -I$(top_srcdir)/include
if ENABLE_RPATH
wvtest_LDFLAGS = -rpath $(libdir)
endif
wvtest_LDADD = $(AM_LDADD) $(top_builddir)/src/libwavpack.la $(LIBM) -lpthread
endif

TESTS = fast-tests
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbry by default make check (which is a target that make distcheck runs, among other checks) runs fast-tests, which is equivalent to running ./wvtest --exhaustive --short --no-extras. If you want to run the full tests:

make check TESTS=all-tests

@SoapGentoo
Copy link
Contributor Author

@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 dbry merged commit d4f7fd2 into dbry:master Apr 26, 2021
@dbry
Copy link
Owner

dbry commented Apr 26, 2021

Thanks!

@SoapGentoo SoapGentoo deleted the autotools-update branch April 26, 2021 07:47
AC_CONFIG_LINKS([
cli/all-tests:cli/all-tests
cli/fast-tests:cli/fast-tests
])
AC_CONFIG_FILES(
Makefile
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbry I'd like to remove the recursive Make build system and move to a non-recursive one. Would you accept a PR to this end?

@dbry
Copy link
Owner

dbry commented Apr 26, 2021

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
Copy link
Contributor Author

  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
Copy link
Owner

dbry commented Apr 27, 2021

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... 😃

@SoapGentoo
Copy link
Contributor Author

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

@dbry
Copy link
Owner

dbry commented Jul 5, 2022

@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
Copy link
Contributor

evpobr commented Jul 5, 2022

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

@SoapGentoo
Copy link
Contributor Author

@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
Copy link
Owner

dbry commented Jul 6, 2022

@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
Copy link
Contributor Author

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 😉 Let me know if you need a fresh bootstrap.

@dbry
Copy link
Owner

dbry commented Jul 6, 2022

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
Copy link
Contributor Author

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
Copy link
Owner

dbry commented Jul 8, 2022

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
Copy link
Contributor Author

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

@dbry
Copy link
Owner

dbry commented Jul 8, 2022

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

Thanks again for all your help!

@SoapGentoo
Copy link
Contributor Author

SoapGentoo commented Jul 9, 2022

@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
Copy link
Owner

dbry commented Jul 9, 2022

@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
Copy link
Owner

dbry commented Nov 26, 2022

@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! 😄

@SoapGentoo
Copy link
Contributor Author

SoapGentoo commented Nov 26, 2022

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

@dbry
Copy link
Owner

dbry commented Nov 26, 2022

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

Thanks!

@SoapGentoo
Copy link
Contributor Author

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

@dbry
Copy link
Owner

dbry commented Nov 27, 2022

Thanks, SHA verified! 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants