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

Use pkg-config in Makefile.simple. #136

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Quuxplusone
Copy link
Contributor

@Quuxplusone Quuxplusone commented Oct 19, 2020

Let's see if TravisCI is happy with this.
Addresses, but does not fully resolve, #135.

EDIT: Nope, GitHub CI's Linux build is not happy with pkg-config sdl. https://github.com/zenorogue/hyperrogue/runs/1276430655

@still-flow
Copy link
Contributor

still-flow commented Nov 2, 2020

Hm, it's on Ubuntu only, it seems. Additionally, plain sdl package seems to be discovered just fine. Could replacing _ -> - in other sdl_-packages help?

According to this, the pkgconfig file is named SDL_mixer.pc. Is it case-sensitive?

@still-flow
Copy link
Contributor

still-flow commented Nov 2, 2020

And sure enough, plain libsdl1.2-dev package provides an sdl.pc file, lowercase. Very consistency.

@Quuxplusone
Copy link
Contributor Author

On my OSX laptop, pkg-config --cflags sdl SDL_mixer works; and pkg-config --cflags SDL sdl_mixer also works; and pkg-config --cflags SDL-mixer does not work. But then, OSX's filesystem is case-insensitive by default.

Just pushed an experiment.

Let's see if TravisCI is happy with this.
Addresses, but does not fully resolve, zenorogue#135.
@Quuxplusone
Copy link
Contributor Author

There, the capitalization is sorted out now. But I'm not keen to merge this half-measure. I'd be interested to hear from @akien-mga whether this is a step in the right direction. What else (if anything) needs to be done to support make -f Makefile.simple on your platform?

@akien-mga
Copy link

I'd be interested to hear from @akien-mga whether this is a step in the right direction. What else (if anything) needs to be done to support make -f Makefile.simple on your platform?

That is a step in the right direction, but what's missing is actually setting the compile flags, and not just linker flags.
i.e. use $(shell pkg-config --cflags sdl SDL_gfx SDL_mixer SDL_ttf) in CXXFLAGS_EARLY, or a new CXXFLAGS_SDL you'd add to the build command.

Note that -pthread is a good practice to link with libpthread with GCC / Clang, but I think it should also be passed as compile flags, since it affects both the compile and linker flags: https://stackoverflow.com/questions/2127797/significance-of-pthread-flag-when-compiling

@still-flow
Copy link
Contributor

FWIW, I wanted to say a bit about the original issue, #135. Compare the #include directives in SDL_gfxPrimitives.h, taken from what I believe to be upstream, and in e.g. SDL_ttf.h. <SDL.h> in former vs. "SDL.h" in latter. The thing is, on all Unix distributions I'm aware of, all SDL headers lie in SDL subdirectory of the standard include path. So the former IIRC wouldn't work without custom -I switches to the compiler.

@akien-mga
Copy link

@still-flow Yes, and -I/usr/include/SDL (or equivalent depending on where things were installed) is one of the things that the advised addition of $(shell pkg-config --cflags sdl SDL_gfx SDL_mixer SDL_ttf) to CXXFLAGS would yield.

@still-flow
Copy link
Contributor

Sorry, I was probably stating the obvious (:. I don't disagree, I guess I just half-heartedly tried to say that this was an upstream issue. But upstream doesn't look like it's going to be updated anytime soon, so probably this needs fixing here.

@akien-mga
Copy link

akien-mga commented Nov 8, 2020

It's not an upstream issue. Upstream provides you the relevant information via pkg-config and sdl-config. It's the responsibility of downstream users to set their include paths accordingly, just like you're also responsible for explicitly linking the libraries that you use.

That's why pkg-config exists, it provides you information from upstream on how to use their code downstream:

pkg-config --cflags --libs sdl SDL_gfx SDL_mixer SDL_ttf
-I/usr/include/SDL -D_GNU_SOURCE=1 -D_REENTRANT -lSDL_gfx -lSDL_mixer -lSDL_ttf -lSDL 

BTW, I would advise to change hyperrogue's own includes #include <SDL/SDL.h> to #include <SDL.h> once the include path has properly been set to point to the SDL headers install location.

@Quuxplusone
Copy link
Contributor Author

My two cents: You're both (at least partly) right.

  • @akien-mga is right that this PR is incomplete; it should really use pkg-config for the --cflags as well. I'm planning to update this PR, but it is low on my to-do list at the moment, and if someone wanted to suggest the exact diff, that'd be cool.

  • I think @akien-mga is right that if we used pkg-config for both sdl and SDL_gfxPrimitives, then the upstream code in "SDL_gfxPrimitives.h" would work okay. I'm about 90% sure of this.

  • @still-flow is right that "SDL_gfxPrimitives.h" currently uses the wrong kind of #include directive. This is a defect in the upstream code. I've known about this for years and years, having seen it in I-forget-what project. The correct way for anyone to include SDL headers is #include <SDL/whatever.h>; that's how everyone in the world does it except for this one "SDL_gfxPrimitives.h" file. Someone else with the same problem. I have vague recollections of emailing Andreas Schiffler about this, years ago, and getting a response maybe along the lines of "these headers are no longer maintained because SDL2 makes this all obsolete." Oddly, he's in my Gmail address book, but I see no archived emails to or from; I have no evidence for my recollections.

@still-flow
Copy link
Contributor

Thank you both for explaining. So from all of this, it seems like the best solution would be to ditch SDL1.2 altogether, got it (;. Well, the official migration guide is encouragingly not too long, so one of these days I might try going through it and seeing where it leads. (But I wouldn't suggest anyone hold their breath on that, so all of the above still applies.)

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.

None yet

3 participants