Extend travis build matrix to include bundled

[reynisdrangar]

Doubles the number of builds, unfortunately, but it should
cover all the common linkage scenarios, except for macos
frameworks, which I'm not sure I know enough about to handle.

Also autoformats travis.yml and splits the SDL archive extraction
and installation out to a shell script.

Travis build result: https://travis-ci.org/reynisdrangar/rust-sdl2/builds/401276819

[reynisdrangar]

Script is shellcheck-clean. Adding the static linkage feature flag introduces the failure from 777, we can toss that back in once that issue is fixed.

Version upgrades for the "system-installed SDL" scenario should be pretty much just tweaking the shellscript. If you'd like it moved to a variable or somesuch I'm happy to do that too.

[Cobrand]

Could you rebase on master? (the only change you should have to do is to update from 2.0.5 to 2.0.8)

[Cobrand]

Why did you remove the features "gfx image ttf mixer" from the doc upload?

[reynisdrangar]

Typo, sorry! Fixing.

[reynisdrangar]

Ah, looks like that's how that one was before:

 after_success:
-- travis-cargo --only stable doc-upload
-env:
-  global:
-  - RUST_TEST_THREADS=1
-  - TRAVIS_CARGO_NIGHTLY_FEATURE=""
-  - LD_LIBRARY_PATH: "/usr/local/lib"
-  - secure: MJhmVnQ2IM7+sVmc3vU4ndKOcQgLLeHUPW3qaQBQHKQmvoswCwQK60N17uSgWn1Ln8teqvSRHq4KclIjdMHI+VuQXJHQKHDgjcYbHxwmc3AM1Whnp0XB44ksKUmD109BGWSfZQxzF+6dA+YNOQ+mti+bpydMu8n2FMVjA/SXwQ8=
+  - travis-cargo --only stable doc-upload

[Cobrand]

Right, my mistake. doc-upload takes no parameter, but doc takes -- --features "gfx image ttf mixer" and you properly did that.

[reynisdrangar]

Rebased on master -- apologies for the autoformatter changes on the travis spec making it harder to review than it has to be.

[Cobrand]

It looks like it's failing on mac with the bundled feature. That may have been the rebase that caused this though: https://travis-ci.org/Rust-SDL2/rust-sdl2/jobs/401554923 (not your actual rebase but the huge code/build.rs change that were made in between)

[Cobrand]

Can someone help with this one? Testing with travis is just a hassle and I don't have a mac handy...

[reynisdrangar]

Hey, I'll take it, if half a dozen lines of yaml can catch a regression, so much the better ;-).

I'll try to take a look at that failure this evening

[reynisdrangar]

Ok, remaining trouble spots on macos, as far as I've found:

Test Failure

On my first build of upstream master I got a failure on the events test, but I couldn't repro it on subsequent runs with RUST_BACKTRACE set. Guessing from the failure this is a race condition in the test (upstream SDL_PushEvent maybe resets the timestamp?), since it's a timestamp field causing a structure comparison assert to bomb out. If I'm right about that, the fix is to modify the test to ignore differences in that field.

Failure log: https://gist.github.com/reynisdrangar/37053162c50df9bd80c8764d70331825

Build Failure

It looks like the linkage issue here comes from a change in dylib naming — upstream seems to have changed something about the symlink/dylib naming such that what we're putting in out/lib is a broken link rather than the actual shared library.

Getting in the directory and building it myself seems to correctly yield the dylib, so next up I need to sort out exactly what cmake invocation the build.rs cmake object constructs.

[Cobrand]

Need any help from other OSX contributors?

[reynisdrangar]

I won't turn it down (I'm on the road visiting family this week, hence the slow replies, my apologies).

I patched build.rs from sdl2-sys with cfg.out_dir("/tmp/sdl2-build"); to get a clean isolated path to peek at, and it looks like my guess from earlier was about on target:

/t/sdl2-build ❯❯❯ find . | grep dylib | xargs file
./lib/libSDL2.dylib:   broken symbolic link to libSDL2-2.0.dylib
./build/libSDL2.dylib: broken symbolic link to libSDL2-2.0.dylib

Rebuilding directly with cmake in this tree gets a dylib in the build path:

/t/sdl2-build ❯❯❯ cmake --build build                                         ⏎
[  1%] Linking C shared library libSDL2.dylib
[ 98%] Built target SDL2
[100%] Built target SDL2main
/t/sdl2-build ❯❯❯ find . | grep dylib | xargs file
./lib/libSDL2.dylib:       broken symbolic link to libSDL2-2.0.dylib
./build/libSDL2.dylib:     Mach-O 64-bit dynamically linked shared library x86_64
./build/libSDL2-2.0.dylib: Mach-O 64-bit dynamically linked shared library x86_64

I'm guessing the upstream cmake script needs a tweak.

[reynisdrangar]

Applying this change to upstream (SDL) seems to fix the problem:

    set_target_properties(SDL2 PROPERTIES
      OUTPUT_NAME "SDL2-${LT_RELEASE}"
      MACOSX_RPATH 1)

[Cobrand]

@reynisdrangar Can you test your PR on macos with SDL2 2.0.5 instead of 2.0.8? You basically just have to change those lines: https://github.com/Rust-SDL2/rust-sdl2/blob/master/sdl2-sys/build.rs#L23-L26

Obviously I'd prefer if upstream was to be fixed, but I'd like this PR to be merged ASAP so we can't wait that long. There's that, plus the fact that since 2.0.5 I feel like SDL2 has been kinda unstable, so I wouldn't mind if we kept the default of using 2.0.5 for "bundled" feature.

[pyrrho]

OSX user here. Who might have an idea of what's going on, actually.

SDL2 2.0.6 introduced a bug in their CMake build system for OSX. The tl;dr is that cmake --build . --target install clobbers the built libSDL2.dylib with a symlink to itself. I have a bug report and a patch open to fix that issue in SDL2, but it looks like it's not yet been seen by the maintainers.
That said, you're either going to have lock into 2.0.5 or fix the SDL2 CMakeLists.txt as part of the sdl2-sys build process to get things linking on OSX.

I did a local build of this branch with the changes recommended by @Cobrand, and I'm getting a failure in the audio::test::test_audio_cvt. I've already pinged I'm trying to figure out how to ping @reynisdrangar to see if I can get access to his fork of this project so that I can rebase this PR onto master, lock in 2.0.5, and see if that fixes this test failure.

[Cobrand]

@pyrrho Thanks for the explanation. If he doesn't answer in a few days, you could always rebase the branch locally and then create another PR that would replace this one.

We would then merge yours and close this one, but reynisdrangar would still be referenced in the commits pushed so I don't think he will mind.

[pyrrho]

I never did get directly in touch, but he added me as a contributor to his fork 😄 so I'm going to guess he won't mind me making some changes there. Thanks @reynisdrangar!

[pyrrho]

So I've rebased, switch back to SDL2 2.0.5, and (potentially) fixed that audio failure, but I'm not sure this should be merged in.

I switched over to my windows system, and I'm seeing a linker failure on windows in this branch;

  = note: libsdl2-63bdc3318a524453.rlib(sdl2-63bdc3318a524453.25mb1p8d9mkopkfw.rcgu.o) : error LNK2019: unresolved external symbol SDL_Vulkan_GetInstanceExtensions referenced in function _ZN4sdl25video6Window26vulkan_instance_extensions17hf41fd2be2676973eE
          libsdl2-63bdc3318a524453.rlib(sdl2-63bdc3318a524453.25mb1p8d9mkopkfw.rcgu.o) : error LNK2019: unresolved external symbol SDL_Vulkan_CreateSurface referenced in function _ZN4sdl25video6Window21vulkan_create_surface17h8f56452978e2a71dE
          libsdl2-63bdc3318a524453.rlib(sdl2-63bdc3318a524453.25mb1p8d9mkopkfw.rcgu.o) : error LNK2019: unresolved external symbol SDL_Vulkan_GetDrawableSize referenced in function _ZN4sdl25video6Window20vulkan_drawable_size17h04ecf4554196de43E
          libsdl2-63bdc3318a524453.rlib(sdl2-63bdc3318a524453.rbi4cajrrges6at.rcgu.o) : error LNK2019: unresolved external symbol SDL_Vulkan_LoadLibrary referenced in function _ZN4sdl25video43_$LT$impl$u20$sdl2..sdl..VideoSubsystem$GT$27vulkan_load_library_default17h251cdea957fe57fcE
          libsdl2-63bdc3318a524453.rlib(sdl2-63bdc3318a524453.rbi4cajrrges6at.rcgu.o) : error LNK2019: unresolved external symbol SDL_Vulkan_UnloadLibrary referenced in function _ZN4sdl25video43_$LT$impl$u20$sdl2..sdl..VideoSubsystem$GT$21vulkan_unload_library17hcba742afdcf1727cE
          libsdl2-63bdc3318a524453.rlib(sdl2-63bdc3318a524453.rbi4cajrrges6at.rcgu.o) : error LNK2019: unresolved external symbol SDL_Vulkan_GetVkGetInstanceProcAddr referenced in function _ZN4sdl25video43_$LT$impl$u20$sdl2..sdl..VideoSubsystem$GT$32vulkan_get_proc_address_function17h1bdbf18ee037756fE
          C:\Users\drewp\repos\reynisdrangar-sdl2\target\debug\examples\keyboard_state-c6cd23c04c42420d.exe : fatal error LNK1120: 6 unresolved externals

This makes some sense to me; the Vulkan symbols were added in SDL2 2.0.8, so those symbols won't exist if we build 2.0.5.

I went ahead and tried the windows build on master and the build succeeds. Running the tests fails, though. It looks like the SDL libs aren't being found at runtime. Might be an install path issue? I'm going to dig into this some more to make sure I understand what's going on, but I'd recommend not merging this PR.

[Cobrand]

This makes some sense to me; the Vulkan symbols were added in SDL2 2.0.8, so those symbols won't exist if we build 2.0.5.

With what kind of code did you try this? If you don't use the Vulkan features, the Vulkan functions shouldn't be used (I think?), so you shouldn't have this problem. Does this happen for the "simple" example for instance? If it does, we'll have to make sure vulkan-related functions are feature gated somehow.

[pyrrho]

This was just a simple cargo test --features bundled with reynisdrangar:master checked out.

I'm not entirely familiar with how this code was generated, but my understanding is that as of #785, SDL2 2.0.8 C/++ code was passed through bindgen to generate the sdl[_*]_bindings.rs files. If that's the case, it's no surprise there are some symbol discrepancies when trying to link code that's expecting SDL 2.0.8 symbols to a SDL 2.0.5 library.

I'm not sure if it's feasible to feature-gate code that's inside of machine-generated code, but I could take a look at doing that. I think my first attempt will be to pull down the SDL2 CMakeLists.txt patch I linked to previously and apply that as part of the 2.0.8 build process.

[Cobrand]

I'm not sure if it's feasible to feature-gate code that's inside of machine-generated code, but I could take a look at doing that. I think my first attempt will be to pull down the SDL2 CMakeLists.txt patch I linked to previously and apply that as part of the 2.0.8 build process.

I think bindgen-generated code (the extern "C" fn stuff) is not actually linked unless it's used by actual rust code (non-extern). So the idea would be to feature gate the code that uses vulkan in rust-sdl2, (not -sys) and it should work then. However I'm absolutely NOT sure it works that way, so we need to test it first.

[Cobrand]

@pyrrho Are you willing to work on this soon (the feature gate of Vulkan)? If you don't have time or anything else, please tell me, I'll open a similar PR based on all your commits and fix this whenever I can.

[pyrrho]

@Cobrand I doubt that I'll have time to start work on feature-gating Vulkan for at least a week. I would like to try and implement that gate as a way to get more familiar with this repository (and bindgen in general), but I'd recommend not waiting on me for that addition if it's blocking someone/something else.

That said, I'm fairly certain PR #797 removes the immediate need for that gate. It builds, links, and tests cleanly on both Mac OS and Windows, and there are no longer any missing Vulkan symbols because it gets SDL2 2.0.8 to work. It might be sufficient to close this PR without merging, merge in #797, and open an issue to make sure the feature gate added in a new PR? Assuming the new logic in #797 is an acceptable, that is.