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

Don't hard code header locations, use <SDL2/SDL.h> #7

Merged
merged 1 commit into from
Sep 10, 2021

Conversation

pcbeard
Copy link
Contributor

@pcbeard pcbeard commented Sep 10, 2021

This gives us the flexibility of finding headers wherever they
maybe installed. Currently I'm creating symlinks to the homebrew
build of libSDL2 /usr/local/include and /usr/local/lib.

Description

To be able to build against headers installed EITHER in /usr/include OR /usr/local/{include,lib}, simply reference <SDL2/SDL_*.h>. On Linux, this means the headers will be found in /usr/include if they SDL development package is installed via apt (on Debian), or in /usr/local/include (suitably symlinked) when using some alternative package manager such as Homebrew.

Detailed Design

No new APIs, just some header file changes.

Documentation

How has the new feature been documented?
Have the relevant portions of the guides in the Documentation folder been updated in addition to symbol-level documentation?

Testing

*How is the new feature tested?

Tested building on WSL Debian distribution.

Performance

N/A

Source Impact

It should simplify things.

Checklist

  • I've read the Contribution Guidelines
  • I've followed the coding style of the rest of the project.
  • I've added tests covering all new code paths my change adds to the project (to the extent possible).
  • I've added benchmarks covering new functionality (if appropriate).
  • I've verified that my change does not break any existing tests or introduce unexpected benchmark regressions.
  • I've updated the documentation (if appropriate).

@pcbeard pcbeard requested a review from ctreffs as a code owner September 10, 2021 09:07
@ctreffs
Copy link
Owner

ctreffs commented Sep 10, 2021

Great addition - let's see what the pipeline says then we merge it 🙂
Thank you for your contribution and the good explanation 👍

@ctreffs
Copy link
Owner

ctreffs commented Sep 10, 2021

@pcbeard could you have a look at the Linux CI - we're getting an error there:

swift test
5
'CSDL2' sdl2.pc: warning: prohibited flag(s): -D_REENTRANT
6
[1/6] Compiling CSDL2Tests CSDL2Tests.swift
7
<module-includes>:1:10: note: in file included from <module-includes>:1:
8
#include "shims.h"
9
         ^
10
/__w/SwiftSDL2/SwiftSDL2/Sources/CSDL2/shims.h:13:11: note: in file included from /__w/SwiftSDL2/SwiftSDL2/Sources/CSDL2/shims.h:13:
11
        #include "other_platforms.h"
12
                 ^
13
/__w/SwiftSDL2/SwiftSDL2/Sources/CSDL2/other_platforms.h:39:10: error: 'SDL2/SDL_sensor.h' file not found
14
#include <SDL2/SDL_sensor.h>
15
         ^
16
/__w/SwiftSDL2/SwiftSDL2/Sources/CSDL2Wrapped/CSDL2Wrapped.swift:9:19: error: could not build C module 'CSDL2'
17
@_exported import CSDL2
18
                  ^
19
error: fatalError
20
make: *** [test] Error 1
21
Makefile:14: recipe for target 'test' failed
22
Error: Process completed with exit code 2.

@ctreffs ctreffs added the enhancement New feature or request label Sep 10, 2021
@pcbeard
Copy link
Contributor Author

pcbeard commented Sep 10, 2021

I looked over the logs, I guess the file <SDL2/SDL_sensor.h> isn't part of the version of SDL2 installed on your builder? If we were building with clang, we could use __has_include(). I will try removing that header from the file to see if that fixes the build. I see in your logs, that sdl2-config --version is printing 2.0.8, and on Debian 10, I'm getting 2.0.9.

This gives us the flexibility of finding headers wherever they
maybe installed. Currently I'm creating symlinks to the homebrew
build of libSDL2 /usr/local/include and /usr/local/lib.
@pcbeard
Copy link
Contributor Author

pcbeard commented Sep 10, 2021

My earlier change was based on copying the apple_macOS.h header file. This time I just modified the current one, which clearly leaves out SDL_sensor.h. I feel like we need a better solution for this. Granted this is a somewhat obscure header file; looks like it's for getting accelerometer data, but this header file does exist in v2.0.9.

@ctreffs ctreffs enabled auto-merge (squash) September 10, 2021 11:38
@ctreffs ctreffs disabled auto-merge September 10, 2021 11:40
@ctreffs ctreffs merged commit 23984cf into ctreffs:master Sep 10, 2021
@pcbeard pcbeard deleted the Homebrew branch September 10, 2021 23:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants