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

Replace mercurial's tar.bz2 with the primary tar.gz #4

Merged
merged 9 commits into from
Sep 6, 2018
Merged

Replace mercurial's tar.bz2 with the primary tar.gz #4

merged 9 commits into from
Sep 6, 2018

Conversation

kcgen
Copy link
Contributor

@kcgen kcgen commented Sep 3, 2018

Solves WohlSoft/SDL-Mixer-X#21, which is due a flaw in Mercurial's generated tar.bz2 file.

Using SDL2's tar.gz file now extracts cleanly:

<snip>
-- [download 100% complete]
-- Downloading... done
-- extracting...
     src='/usr/src/SDL-Mixer-X/build/external/AudioCodecs/src/AudioCodecs-build/external/SDL2/src/SDL2-2.0.8.tar.gz'
     dst='/usr/src/SDL-Mixer-X/build/external/AudioCodecs/src/AudioCodecs-build/external/SDL2/src/SDL2HG'
-- extracting... [tar xfz]
-- extracting... [analysis]
-- extracting... [rename]
-- extracting... [clean up]
-- extracting... done
[  1%] No patch step for 'SDL2HG'
[  1%] No update step for 'SDL2HG'
[  1%] Performing configure step for 'SDL2HG'
-- The C compiler identification is GNU 7.3.0
<snip>

Attempting to solve WohlSoft/SDL-Mixer-X#21, which is due to cmake's internal tar-extractor failing on the bz2 file.
@Wohlstand
Copy link
Member

Please make it to be an option to choose which version of SDL2 to pull: stable release or dev one from HG. Myself I using most fresh SDL2 as sometimes I finding bugs in it and I posting fixes of them to SDL2 team and there are accepting them.

Look to my AppVeyor builds are using MinGW-w64 and successfully compiling, pulling, unpacking SDL2 from HG. I bet you have forgot to install bzip2 into your msys2...

@@ -42,6 +43,6 @@ install(
CODE "file( INSTALL \${builtSdl2Heads} DESTINATION \"${CMAKE_INSTALL_PREFIX}/include/SDL2\" )"
CODE "file( GLOB builtSdlLibs \"${CMAKE_BINARY_DIR}/lib/*SDL2*\" )"
CODE "file( INSTALL \${builtSdlLibs} DESTINATION \"${CMAKE_INSTALL_PREFIX}/lib\" )"
CODE "file( GLOB builtSdlBins \"${CMAKE_BINARY_DIR}/bin/*SDL2*\" )"
CODE "file( GLOB builtSdlBins \"${CMAKE_BINARY_DIR}/bin/*\" )"
Copy link
Member

Choose a reason for hiding this comment

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

It's bad idea: every other library that included into this AudioCodecs pack is adding into same install path. so, making this you making duplicates be installed which will overflow the queue...

@@ -26,11 +26,12 @@ endif()
ExternalProject_Add(
SDL2HG
PREFIX ${CMAKE_BINARY_DIR}/external/SDL2
URL https://hg.libsdl.org/SDL/archive/default.tar.bz2
URL https://www.libsdl.org/release/SDL2-2.0.8.tar.gz
Copy link
Member

Choose a reason for hiding this comment

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

Why not to make the option:

option(SDL_USE_RELEASE ...)
if(SDL_USE_RELEASE)
    set(SDL_TAR_URL "https://www.libsdl.org/release/SDL2-2.0.8.tar.gz")
else()
    set(SDL_TAR_URL  "https://hg.libsdl.org/SDL/archive/default.tar.bz2")
endif()

It's better to give a choice than force one of...

@@ -26,7 +27,7 @@ endif()
ExternalProject_Add(
SDL2HG
PREFIX ${CMAKE_BINARY_DIR}/external/SDL2
URL https://www.libsdl.org/release/SDL2-2.0.8.tar.gz
GIT_REPOSITORY https://github.com/spurious/SDL-mirror.git
Copy link
Member

Choose a reason for hiding this comment

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

Please keep old URL as comment:

# FIXME: Return back old URL when SDL2 team will fix this issue: https://bugzilla.libsdl.org/show_bug.cgi?id=4248
# URL https://hg.libsdl.org/SDL/archive/default.tar.bz2

CMAKE_ARGS
"-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}"
"-DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}"
-DSNDIO=OFF
-DSDL_SHARED=${SHARED}
Copy link
Member

Choose a reason for hiding this comment

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

Let's this will be a part of this?
#5

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, as it's undefined now, it will cause an error... Please define next options:

option(AudioCodecs_SDL2_STATIC ...)
option(AudioCodecs_SDL2_SHARED ...)

which are both will be ON by default.
Then, pass them as:

-DSDL_SHARED=${AudioCodecs_SDL2_SHARED}
-DSDL_STATIC=${AudioCodecs_SDL2_STATIC}

@@ -27,7 +27,8 @@ endif()
ExternalProject_Add(
SDL2HG
PREFIX ${CMAKE_BINARY_DIR}/external/SDL2
GIT_REPOSITORY https://github.com/spurious/SDL-mirror.git
URL https://hg.libsdl.org/SDL/archive/default.tar.bz2
# GIT_REPOSITORY https://github.com/spurious/SDL-mirror.git
Copy link
Member

Choose a reason for hiding this comment

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

LOL 🤣
Are you wanted to put GIT repo in a top? I have asked you to keep old URL as comment, but you are did opposite: you have kept old URL but temporary GIT as comment! 🤣 🦊

@Wohlstand
Copy link
Member

Okay, tomorrow I'll take this for polishing and then merge after set of changes... For now I have tired after a hard day.

CMAKE_ARGS
"-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}"
"-DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}"
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
Copy link
Member

Choose a reason for hiding this comment

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

The reason why those are quoted - ability to have space-contained paths. Removal of quotes breaks the support of them.

@Wohlstand Wohlstand merged commit b514c8f into WohlSoft:master Sep 6, 2018
@Wohlstand
Copy link
Member

I have merged this with some of changes I have made by myself. I have added two options:

  • BUILD_SDL2_SHARED where you can enable or disable shared build of downloading SDL2
  • BUILD_SDL2_STATIC where you can enable or disable static build of downloading SDL2

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.

2 participants