Skip to content

More automated testing#249

Merged
slouken merged 4 commits intolibsdl-org:mainfrom
smcv:more-testing
May 20, 2022
Merged

More automated testing#249
slouken merged 4 commits intolibsdl-org:mainfrom
smcv:more-testing

Conversation

@smcv
Copy link
Copy Markdown
Contributor

@smcv smcv commented May 20, 2022

  • workflows: Collect artifacts for all jobs, not just Linux

  • workflows: Exercise static linking with CMake

    There are enough subtleties with the vendored libraries that it seems
    worthwhile to test this.

  • cmake: Add a shortcut for setting most dependencies _SHARED or not

  • workflows: Run the automated test for CMake builds

    We can't use dlopen() for the dependencies, because they won't be in
    the search path at runtime, so link them statically here.

smcv added 4 commits May 20, 2022 18:53
Signed-off-by: Simon McVittie <smcv@collabora.com>
There are enough subtleties with the vendored libraries that it seems
worthwhile to test this.

Signed-off-by: Simon McVittie <smcv@collabora.com>
Signed-off-by: Simon McVittie <smcv@collabora.com>
We can't use dlopen() for the dependencies, because they won't be in
the search path at runtime, so link them statically here.

Signed-off-by: Simon McVittie <smcv@collabora.com>
@smcv
Copy link
Copy Markdown
Contributor Author

smcv commented May 20, 2022

Ugh, sorry, I should have marked that as a draft - it's somewhat broken.

@slouken
Copy link
Copy Markdown
Collaborator

slouken commented May 20, 2022

Oops. I'll ignore test failures for the moment then.

run: |
set -eu
rm -fr DESTDIR-cmake
DESTDIR=$(pwd)/DESTDIR-cmake cmake --install build/ --config Release
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, I would create a tarball of the install prefix, and add it to the artifacts (for both autotools and cmake).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For what purpose? These binaries are not really expected to work on any system other than the one they were built on (which is essentially arbitrary).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Convenience binaries for allowing people testing latest master without them needing to rebuild SDL_image.
But your reasoning sounds solid.

option(BUILD_SHARED_LIBS "Build the library as a shared library" ON)

option(SDL2IMAGE_INSTALL "Enable SDL2_image install target" ON)
option(SDL2IMAGE_DEPS_SHARED "Load dependencies dynamically" ON)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the things I was not sure about in the cmake build script:
does it make sense to dynamically load only certain dependencies?

Currently, this variable is used as a default variable for cmake_dependent°option.
Personally, I would ditch all SDL2IMAGE_xxx_SHARED options and only use this one variable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense to dynamically load only certain dependencies?

I don't see why not: a Linux distribution that is less hardline about normal linking than Debian might want to make libjpeg and libpng be hard dependencies, but make libavif and libjxl weak/optional dependencies.

@smcv
Copy link
Copy Markdown
Contributor Author

smcv commented May 20, 2022

Ugh, sorry, I should have marked that as a draft - it's somewhat broken.

#251 resolves that, and I'll have another try another time. Sorry about that.

Now that we're starting to have meaningful CI, it's probably best to wait for it to pass before hitting merge.

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