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 #9

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

hanetzer
Copy link
Contributor

@hanetzer hanetzer commented Jan 8, 2018

There, completely overhauled and working for mingw-w64 and linux.

@hanetzer hanetzer mentioned this pull request Jan 8, 2018
@Ancurio
Copy link
Owner

Ancurio commented Jan 8, 2018

thanks, I'll try these out when I get home

@hanetzer
Copy link
Contributor Author

hanetzer commented Jan 8, 2018

K. Don't merge it here yet; I've been doing a bit more research, and while this works,
I should move the library assignments into LIBS and not LDFLAGS.

@hanetzer hanetzer force-pushed the autotools-update branch 3 times, most recently from 11637c1 to 6298e75 Compare January 8, 2018 20:28
@hanetzer
Copy link
Contributor Author

hanetzer commented Jan 8, 2018

There we go. Should be good to go for a merge now.

@hanetzer hanetzer force-pushed the autotools-update branch 3 times, most recently from 4c0e528 to aaa4dff Compare January 9, 2018 01:02
Current autotools/autoconf uses configure.am by default, not
configure.in.

Signed-off-by: Marty E. Plummer <[email protected]>
Signed-off-by: Marty E. Plummer <[email protected]>
Signed-off-by: Marty E. Plummer <[email protected]>
Signed-off-by: Marty E. Plummer <[email protected]>
One only needs -lSDL2main and -mwindows when building an application
linked against SDL2, not a library which is linked against SDL2.
Without this change you'll only be able to build a static libSDL_sound.a
because libtool freaks over libmingw32.a and libSDL2main.a not having a
real library/dll to go with them.

Signed-off-by: Marty E. Plummer <[email protected]>
@hanetzer
Copy link
Contributor Author

hanetzer commented Jan 9, 2018

Actually hold off just a bit more; I need to get SDL_VERSION back into the mix.

Needed a local patch to pkg.m4 to include the found version of a
pkg-config module; will be sending this upstream as well, because it
would be useful to be able to query a returned version from
PKG_CHECK_MODULES.

Signed-off-by: Marty E. Plummer <[email protected]>
@hanetzer
Copy link
Contributor Author

hanetzer commented Jan 9, 2018

Alrighty. Had to patch pkg.m4, which is part of the pkg-config package, to produce $PKG_VERSION variables. I've filed a bug with freedesktop and attached my patch as an example (they may want
to be a bit more verbose/cautious with how they implement it, but this works for now), so its good
to go for our purposes, for now.

@Ancurio
Copy link
Owner

Ancurio commented Jan 17, 2018

Three things you might add while you're at it:

  • replace the bootstrap script with something more mainstream (I think you usually see a autogen.sh or something, right?). I used these commands to make it work: https://askubuntu.com/a/27679

  • If possible, lower the required automake version to 1.14; this is just personal since I don't have 1.15 installed on my old dev machine ;)

  • For some reason it tries to compile the modplug module even though pkg-config couldn't find it as installed (and thus the compilation errors out on the missing header)

Otherwise, great work! It's very nice to have ogg/vorbis detection "just work" via pkg-config.

@hanetzer
Copy link
Contributor Author

honestly bootstrap and autogen.sh are both fairly common, and are primarily just a shortcut so
developers don't have to remember exactly what autotools commands they need to run to regen
the configure and Makefile.in scripts.
I'll see what the deal with modplug, I prolly just misplaced a variable/test as compared to the ones
which worked.

@hanetzer
Copy link
Contributor Author

@Ancurio and could you provide a log of it doing the modplug check/compile (both
configure and make, if possible).

@Ancurio
Copy link
Owner

Ancurio commented Jan 19, 2018

config.log

config.status.log

configure.output.log
(./configure terminal output)

make.output.log
(make terminal output)

Makefile.txt
(Makefile)

@hanetzer
Copy link
Contributor Author

hanetzer commented Jan 20, 2018

@Ancurio ok, having a look at it. For the record, do you actually have modplug present in deps-win32?
And is it present in your native linux? (That is, could it be finding the linux modplug pkg-config file?)

@carstene1ns
Copy link

One big problem here is that the flags and directories pkg-config finds are not used later in the checks of header/library usability, but instead default search directories are used. Which means it cannot be found, if not installed there (mostly on the host system or mingw specific path, e.g. /usr/i686-w64-mingw32/sys-root/mingw/lib/pkgconfig in this case).

It bothers me that you kept most of the old stuff and just changed the formatting.
If you use pkg-config, then rely on it. That means drop the old header/library detection, as nowadays there are very few platforms that could benefit from it anyway (if any).
Do not bend the LIBS and *FLAGS, if you do not have to. Use the variables pkg-config gives you instead:
libSDL_sound_la_CFLAGS += $(BLA_CFLAGS) and libSDL_sound_la_LIBADD += $(BLA_LIBS) which is much cleaner as you can only add them where needed instead of globally.

@Ancurio
Copy link
Owner

Ancurio commented Jan 20, 2018

@hanetzer Nvm I'm an idiot, I do have modplug installed under my deps-win32 prefix. Sorry for the noise.

And I am very confused. Both libmodplug.pc files (native installed and in my deps-win32 prefix) just list the topl [...]/include/ as include directory, however modplug.h is always in [...]/include/libmodplug/. So either the .pc files are broken, or the expected way of including is #include <libmodplug/modplug.h>. Which raises the question: How in the hell did this part ever compile in the first place on my machine??

@carstene1ns
Copy link

carstene1ns commented Jan 20, 2018

The header directory change happened with libmodplug 0.8.8.5 release. It has hit almost all projects using it, though and AFAIR the reason for that breaking change was never explained.

The configure script actually checks this case:
https://github.com/Ancurio/SDL_sound/blob/master/configure.in#L208-L211

@hanetzer
Copy link
Contributor Author

hanetzer commented Jan 20, 2018

@carstene1ns I do check that as well, and to be frank they should add it to the pkg-config file
so such a check is not needed.

Also, using the APPEND_FLAGS stuff actually checks if the compiler/linker can actually use those
flags, instead of blindly appending them to libSDL_sound_la_STUFF.

@carstene1ns
Copy link

Libraries should only ever ship with flags in their pkg-config files, that are needed to compile against them, so testing should not really be needed.
I agree that the include directory change for the modplug header was bad, but it was a decision by their maintainers, so I respect that.
However, my claim is still: AC_CHECK_HEADER will not use the path pkg-config found, so this fails.

@Ancurio
Copy link
Owner

Ancurio commented Jan 20, 2018

Ok so for the modplug issue, it's probably easiest to just change the include directive (the breaking change happened in 2014, so I don't feel confident the pc file is going to get fixed any time soon).

So I thought about updating the bootstrap script myself; hopefully the OSX specific glibtool part will not be broken by that..

Any comment on lowering the min version to 1.14?

@hanetzer
Copy link
Contributor Author

@Ancurio I could, care to edit it locally and test it?

I'm also planning to add a CMake project as well, makes things a bit easier.

@Ancurio
Copy link
Owner

Ancurio commented Jan 20, 2018

@hanetzer I already did locally, otherwise I couldn't have built it in the first place :) Didn't see any obvious problems.

@hanetzer
Copy link
Contributor Author

@Ancurio would you be alright with dropping support for libmodplug before the header movement?

@Ancurio
Copy link
Owner

Ancurio commented Feb 24, 2018

@hanetzer You mean the versions of libmodplug prior to the header change? Yeah sure.

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