Add CMake support by FtZPetruska · Pull Request #48 · libsdl-org/SDL_net

FtZPetruska · 2022-05-29T06:33:59Z

Description

This PR adds support CMake, based on SDL2_image's CMakeLists.

This is a follow-up to #37 which was lacking many features (incomplete Config file, missing FindSDL2 helpers, etc.).

Additionally, the showinterfaces program is supported.

Existing issues

Addresses #33.

Implementation is based off SDL2_Image.

madebr

Do you intend to add CMake to the GitHub workflows?

CMakeLists.txt

SDL2_netConfig.cmake.in

Changes link flags to fix mingw builds. `showinterfaces` is disabled on MSVC workflow as the minimum version of SDL is too old for current MSVCRT and gives linker errors even when linking `legacy_stdio_definitions.lib`

FtZPetruska · 2022-05-29T22:04:30Z

Thank you for your review!

Do you intend to add CMake to the GitHub workflows?

I went ahead and added them in 553058d.

madebr · 2022-05-29T22:07:24Z

Thank you for adding them!
Weird that legacy_stdio_definitions.libdoes not fix the MSVC link error.
It looks very similar (if not the same) as the one I was seeing.

FtZPetruska · 2022-05-29T22:21:50Z

I was testing with SDL2.0.0 as it is the minimum compatible version. The two errors I was getting were undefined references in SDL2main.lib(SDL_windows_main.obj) within the ShowError function to:

__imp_fprintf
__imp___iob_func

Linking to legacy_sdtio_definitions.lib would only fixes the fprintf issue.

On the other hand, bumping the version to SDL2.0.4 fixes this issue altogether.

madebr · 2022-05-29T22:28:48Z

That sounds credible.
I only had to go back to SDL2.0.9, so you were seeing "bigger" issues.
It looks like we're seeing the same issue as this stackoverflow question.

This is what msdn has to say about this:

If your project links with static libraries that were compiled with a release of Visual Studio earlier than 2015, the linker might report an unresolved external symbol. These errors might reference internal definitions for _iob, _iob_func, or related imports for certain <stdio.h> functions in the form of imp*. Microsoft recommends that you recompile all static libraries with the latest version of the C++ compiler and libraries when you upgrade a project. If the library is a third-party library for which source isn't available, you should either request an updated binary from the third party or encapsulate your usage of that library into a separate DLL that you compile with the older version of the compiler and libraries.

So yeah, good call to just ignore the problem until we update the minimum required SDL version (which is unlikely to happen anytime soon).

sezero · 2022-05-29T22:34:40Z

I was testing with SDL2.0.0 as it is the minimum compatible version. The two errors I was getting were undefined references in SDL2main.lib(SDL_windows_main.obj) within the ShowError function to:
* `__imp_fprintf`

* `__imp___iob_func`
Linking to legacy_sdtio_definitions.lib would only fixes the fprintf issue.

On the other hand, bumping the version to SDL2.0.4 fixes this issue altogether.

Is this issue with linkage of showinterfaces test program? AFAIK, it is a console-only
program, so, does defining SDL_MAIN_HANDLED in its CFLAGS fix things?

Only set `Compatible Interface Properties` for shared libraries. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

madebr

Thanks for investing your time into this!

This removes the need for SDL2main as it is a console-only application. MSVC workflow now builds showinterfaces.

FtZPetruska · 2022-05-29T23:04:08Z

I was testing with SDL2.0.0 as it is the minimum compatible version. The two errors I was getting were undefined references in SDL2main.lib(SDL_windows_main.obj) within the ShowError function to:
* `__imp_fprintf`

* `__imp___iob_func`
Linking to legacy_sdtio_definitions.lib would only fixes the fprintf issue.
On the other hand, bumping the version to SDL2.0.4 fixes this issue altogether.
Is this issue with linkage of showinterfaces test program? AFAIK, it is a console-only program, so, does defining SDL_MAIN_HANDLED in its CFLAGS fix things?

Thank you for the suggestion, it worked like a charm!

CMakeLists.txt

The `mingw32` library is no longer needed as `showinterfaces` is a console app. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

Applied review suggestion to use `target_compile_options`. Replaced `/wd4996` as `_WINSOCK_DEPRECATED_NO_WARNINGS` is enough. Raise the warning level to `/W3` to be consistent with the solution in `VisualC/`.

CMakeLists.txt

slouken · 2022-05-30T14:32:47Z

On the other hand, bumping the version to SDL2.0.4 fixes this issue altogether.

This is the right solution here. SDL_net shouldn't be held back to SDL 2.0.0.

CMakeLists.txt

- Set a different output name for static libraries on MSVC and WATCOM (cf. libsdl-org/SDL_image#275 and libsdl-org/SDL#5727) - Fix `PKG_PREFIX` on Windows (cf. libsdl-org/SDL_image#274) Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

slouken · 2022-05-30T17:55:46Z

@madebr, feel free to merge this and/or incorporate the changes into your other SDL library work.

SDL_net does not build any dependency, so we don't need to force position independent code.

madebr · 2022-06-11T12:23:44Z

Thus far, this PR looks very good!
I'm currently also working on similar changes for SDL_ttf and SDL_mixer (=in progress).

What is missing here is generation of cmake config scripts by the autotools toolchain.
Also addition of cmake scripts that will get bundled into the release for the Xcode framework, the MinGW and VC SDK is missing.
In the PR mentioned above, I also added a mini CMake integration test to the GitHub CI.

The SDL_ttf pr has the missing bits I mentioned above that you would need to adapt to SDL_net.

Are you willing to look into this, or do you prefer to merge and let me finish it?
You doing the port would also mean my SDL_ttf pr getting a review by you.

Thanks!

FtZPetruska · 2022-06-11T13:42:58Z

Thank you for your comment!

I'm going to look into it and I'll let you know how it goes.

I'm away from my main machine currently, so I can only really test autotools and Apple's Framework integration. I'll try to grab mingw binaries when I get the opportunity to test it out.

Xcode/pkg-support/resources/CMake/sdl2_net-config.cmake

madebr

I've created FtZPetruska#2 with fixes to the build.

…cific SDL2 sharedness library

Fix option de-duplication issues when linking with SDL2 framework. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

This is necessary if the framework has a space in its path.

As mentionned in libsdl-org/SDL_net#48 and libsdl-org/SDL_ttf#213: - Options needs to use `SHELL:` to avoid aggressive option de-duplication - Framework path needs to be quoted to support paths with spaces.

mingw/pkg-support/cmake/sdl2_net-config.cmake

Fix name mismatch between the bundled CMake config file for mingw and the generated config. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

sdl2_net-config.cmake.in

Fix regex match. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

slouken · 2022-06-13T13:29:43Z

@madebr, feel free to merge this when ready.

CMakeLists.txt

No longer set PKG_PREFIX on macOS. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

madebr · 2022-06-14T21:55:57Z

Thanks @FtZPetruska !

Add CMake support.

04069fc

Implementation is based off SDL2_Image.

slouken requested a review from madebr May 29, 2022 07:34

madebr reviewed May 29, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

CMakeLists.txt Show resolved Hide resolved

SDL2_netConfig.cmake.in Outdated Show resolved Hide resolved

FtZPetruska and others added 2 commits May 29, 2022 16:14

Add CMake tests to test-versioning.sh.

fd6bc05

Add CMake workflow

553058d

Changes link flags to fix mingw builds. `showinterfaces` is disabled on MSVC workflow as the minimum version of SDL is too old for current MSVCRT and gives linker errors even when linking `legacy_stdio_definitions.lib`

Apply suggestion from review

61ccf8e

Only set `Compatible Interface Properties` for shared libraries. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

madebr approved these changes May 29, 2022

View reviewed changes

CMake: Define SDL_MAIN_HANDLED for showinterfaces.

6004b9b

This removes the need for SDL2main as it is a console-only application. MSVC workflow now builds showinterfaces.

madebr reviewed May 29, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

madebr reviewed May 29, 2022

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

FtZPetruska and others added 2 commits May 29, 2022 19:34

Apply suggestion from review.

918d216

The `mingw32` library is no longer needed as `showinterfaces` is a console app. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

CMake: Update MSVC compiler flags.

90a143d

Applied review suggestion to use `target_compile_options`. Replaced `/wd4996` as `_WINSOCK_DEPRECATED_NO_WARNINGS` is enough. Raise the warning level to `/W3` to be consistent with the solution in `VisualC/`.

madebr reviewed May 30, 2022

View reviewed changes

CMakeLists.txt Show resolved Hide resolved

madebr reviewed May 30, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

FtZPetruska and others added 2 commits May 30, 2022 12:14

CMake: Fix missing parenthesis.

eb0e85d

slouken assigned madebr May 30, 2022

CMake: Remove dead code.

6e90ae0

SDL_net does not build any dependency, so we don't need to force position independent code.

FtZPetruska added 2 commits June 11, 2022 10:42

CMake: Add config files for XCode Frameworks

7ef9b42

CMake: Generate cmake config when using autotools.

be4936a

FtZPetruska and others added 4 commits June 11, 2022 16:49

CMake: Add config files for mingw devel archive.

2c79c92

CMake: Add config files for VC devel archive.

7b8c4b1

Merge branch 'libsdl-org:main' into CMake

78ca5c2

Regenerate autotools file with CMake support.

7baa006

FtZPetruska requested a review from madebr June 11, 2022 21:38

madebr reviewed Jun 11, 2022

View reviewed changes

Xcode/pkg-support/resources/CMake/sdl2_net-config.cmake Outdated Show resolved Hide resolved

madebr reviewed Jun 11, 2022

View reviewed changes

madebr added 3 commits June 11, 2022 22:38

autogen.sh + cmake integration test

6d5749d

Don't force an installed SDL2_net::SDL2_net-static to link with a spe…

11a9715

…cific SDL2 sharedness library

Update PrivateSdlFunctions.cmake

6d1d9a5

FtZPetruska mentioned this pull request Jun 12, 2022

Cmake work libsdl-org/SDL_ttf#213

Merged

FtZPetruska and others added 2 commits June 12, 2022 08:48

Apply suggestion from review.

691403c

Fix option de-duplication issues when linking with SDL2 framework. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

CMake: Escape framework parent path.

03f5378

This is necessary if the framework has a space in its path.

FtZPetruska mentioned this pull request Jun 12, 2022

CMake: Fix XCode CMake config file. libsdl-org/SDL#5787

Merged

madebr reviewed Jun 12, 2022

View reviewed changes

mingw/pkg-support/cmake/sdl2_net-config.cmake Outdated Show resolved Hide resolved

Apply suggestion from review.

9739a52

Fix name mismatch between the bundled CMake config file for mingw and the generated config. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

madebr reviewed Jun 13, 2022

View reviewed changes

sdl2_net-config.cmake.in Outdated Show resolved Hide resolved

Apply suggestion from review.

499bb18

Fix regex match. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

madebr reviewed Jun 13, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

Apply suggestion from review.

8c910a8

No longer set PKG_PREFIX on macOS. Co-authored-by: Anonymous Maarten <madebr@users.noreply.github.com>

madebr merged commit 20f3329 into libsdl-org:main Jun 14, 2022

FtZPetruska mentioned this pull request Jun 16, 2022

CMake: Add options to create an Apple Framework. #51

Closed

FtZPetruska deleted the CMake branch June 16, 2022 01:21

FtZPetruska mentioned this pull request Jun 17, 2022

[sdl2-image/mixer/net/ttf] Update to exported targets names. microsoft/vcpkg#25284

Closed

This was referenced Jun 21, 2022

cmake: Build samples with cmake #56

Merged

[PATCH] Add CMake support #33

Closed

Conversation

FtZPetruska commented May 29, 2022

Description

Existing issues

Uh oh!

madebr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FtZPetruska commented May 29, 2022

Uh oh!

madebr commented May 29, 2022

Uh oh!

FtZPetruska commented May 29, 2022

Uh oh!

madebr commented May 29, 2022

Uh oh!

sezero commented May 29, 2022

Uh oh!

madebr left a comment

Choose a reason for hiding this comment

Uh oh!

FtZPetruska commented May 29, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

slouken commented May 30, 2022

Uh oh!

Uh oh!

slouken commented May 30, 2022

Uh oh!

madebr commented Jun 11, 2022

Uh oh!

FtZPetruska commented Jun 11, 2022

Uh oh!

Uh oh!

madebr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

slouken commented Jun 13, 2022

Uh oh!

Uh oh!

madebr commented Jun 14, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants