search

jeremyevans / aqualung

Advanced music player
http://aqualung.jeremyevans.net
GNU General Public License v2.0
49 stars 15 forks source link

Use platform-dependent AR #5

Closed ony closed 6 years ago

ony commented 6 years ago

Use of platform-prefixed AR tool in case of

./configure --build=x86_64-pc-linux-gnu --host=x86_64-pc-linux-gnu

Some of distros tries to ensure that packages can be configured to use proper platform-prefixed tools to allow those packages to be build for different host from same system. One of such system is Exherbo linux.

jeremyevans commented 6 years ago

This is not my area of expertise, but it looks like it will break building with some older versions of automake (not sure which), From what I've looked up, we may be able to do m4_ifdef([AM_PROG_AR], [AM_PROG_AR]) to work around that. I'm not sure how common the need is for this, but considering this is the first request and aqualung has been around for many years, I'm guessing it is uncommon. Any other developers want provide feedback (@tomszilagyi)?

ony commented 6 years ago

@jeremyevans, hmm... I thought the main idea behind autotools is to prepare configure.sh that can run on any system. M4 macro-language is used to simplify writing of tedious shell script code in a more conciese way without adding new dependency on a machine that will use configure.sh. I.e. for release archive you should already pre-run autotools to generate configure.sh and allow running it even on hosts that have no autotools installed. This implies also automake to generate Makefile.ac from Makefile.am.

Only those who change source for configure.sh (i.e. configure.ac) require autotools to re-generate shell script. And as a developer you probably want to use latest autotools since they should provide you with a configure.sh which is compatible with more systems. Some distros (like Exherbo) may apply patches for configure.ac and then they'll prepare configure.sh using appropriate versions of autotools. For example I have automake with versions range 1.10 (used by 26 packages), 1.11 (42 packages), 1.12 - 1.16 (289 packages) right now.

I looked again through manual:

AM_PROG_AR([act-if-fail])

You must use this macro when you use the archiver in your project, if you want support for unusual archivers such as Microsoft lib. The content of the optional argument is executed if the archiver interface is not recognized; the default action is to abort configure with an error message.

And indeed to be able to build for unusual systems that have no way to build static archive (from automake perspective) you need additional handling. But this problem should exist already, I guess. If you'll follow manual for creating static libraries:

Building a static library is done by compiling all object files, then by invoking ‘$(AR) $(ARFLAGS)’ followed by the name of the library and the list of objects, and finally by calling ‘$(RANLIB)’ on that library. You should call AC_PROG_RANLIB from your configure.ac to define RANLIB (Automake will complain otherwise). You should also call AM_PROG_AR to define AR, in order to support unusual archivers such as Microsoft lib.

I.e. since you are using static lib in decoder you should have AC_PROG_RANLIB and AM_PROG_AR anyway. If you want I can add AC_PROG_RANLIB in this pull-request to make current sources closer to requirements from manual.

jeremyevans commented 6 years ago

Thanks for the additional information. If you could update the pull request for AC_PROG_RANLIB, I guess that makes sense. I'll test after that, and assuming no problems it can be merged.

ony commented 6 years ago

Thanks for the additional information. If you could update the pull request for AC_PROG_RANLIB, I guess that makes sense. I'll test after that, and assuming no problems it can be merged.

I just noticed that it is already there:

# Checks for programs.
AC_PROG_CC
AC_PROG_RANLIB
AC_PROG_CXX

Sorry for misleading reply.

ony commented 6 years ago

I don't see reason to rush unless next release is planned any time soon. So far we have work-around in Exherbo and I already enjoy it running on my system.

That was just additional context from my short experience and lookup to justify this change.

P.S. Thank you for maintaining this project. And thank you for quick replies.

jeremyevans commented 6 years ago

I've tested and this should be fine. Thanks for your patience.

jeremyevans commented 6 years ago

Thanks for your patience, I've tested this and it doesn't cause any issues, so I merged it.