Replace MPG123 with minimp3, replace std::threads with pthread wrapper, seperate vendor folder into vendor and external #1226

Open

erorcun wants to merge 1 commits from erorcun/master into master

erorcun commented (Migrated from github.com)

There is no content yet.

madebr commented (Migrated from github.com)

@erorcun

See https://github.com/erorcun/re3/pull/1 for cmake and conan fixes related to mp3.

@erorcun See https://github.com/erorcun/re3/pull/1 for cmake and conan fixes related to mp3.

madebr (Migrated from github.com) reviewed

src/CMakeLists.txt

madebr (Migrated from github.com) commented

Please don't do this.
minimp3 is an external dependency and can be provided by external package managers, such as conan.

Please don't do this. `minimp3` is an external dependency and can be provided by external package managers, such as conan.

erorcun (Migrated from github.com) reviewed

src/CMakeLists.txt

erorcun (Migrated from github.com) commented

but we just use 2 headers of it and it doesn't depend on any package on OS, won't git submodule download it already?

withmorten commented (Migrated from github.com)

Again, with all due respect, this is a header only library. Adding cmake modules and whatnot for it, even adding it as a submodule defeats the entire point of it being a header only library. Just drop the two headers into the same dir where you need it (same as the ini header only parser) and that's it! Everything else just overcomplicates it!

Again, with all due respect, this is a header only library. Adding cmake modules and whatnot for it, even adding it as a submodule defeats the *entire* point of it being a header only library. Just drop the two headers into the same dir where you need it (same as the ini header only parser) and that's it! Everything else just overcomplicates it!

madebr (Migrated from github.com) reviewed

src/CMakeLists.txt

madebr (Migrated from github.com) commented

That's what I am abstracting with my pr (the Findminimp3.cmake module).

That's what I am abstracting with my pr (the `Findminimp3.cmake` module).

madebr (Migrated from github.com) reviewed

src/CMakeLists.txt

madebr (Migrated from github.com) commented

My point is to separate re3 code and external dependencies: openal/minimp3/glfw/...

madebr commented (Migrated from github.com)

"Modern cmake" is about thinking with targets, not with include directories/link libraries/.
It's just basic cmake, nothing complicated.

You just link to a target, and don't have to do anything extra.
This means no extra definitions, extra include directories, extra libraries sprinkled around in your code.
The same could apply to the ini parser.

I think this is a matter of difference in culture in cmake and premake.

"Modern cmake" is about thinking with targets, not with include directories/link libraries/. It's just basic cmake, nothing complicated. You just link to a target, and don't have to do anything extra. This means no extra definitions, extra include directories, extra libraries sprinkled around in your code. The same could apply to the ini parser. I think this is a matter of difference in culture in cmake and premake.

erorcun commented (Migrated from github.com)

We decided to move ini parser and minimp3 to src/external folder, without registering any module/submodule. Waiting for reviews.

We decided to move ini parser and minimp3 to `src/external` folder, without registering any module/submodule. Waiting for reviews.

madebr (Migrated from github.com) reviewed

cmake/FindMilesSDK.cmake

madebr (Migrated from github.com) commented

Please keep everything miles related.
This is needed for mingw.

Please keep everything miles related. This is needed for mingw.

madebr (Migrated from github.com) reviewed

cmake/FindMilesSDK.cmake

madebr (Migrated from github.com) commented

Do you have any issues with building with miles support?

madebr (Migrated from github.com) reviewed

src/audio/oal/stream.h

madebr (Migrated from github.com) commented

What prompted you to replace c++ std:: locks with pthread?

madebr (Migrated from github.com) reviewed

src/skel/crossplatform.h

madebr (Migrated from github.com) commented

As an alternative to using a custom MSVC specific pthreads implementation, there is also pthreads4win.

As an alternative to using a custom MSVC specific pthreads implementation, there is also [pthreads4win](https://sourceforge.net/projects/pthreads4w/).

withmorten (Migrated from github.com) reviewed

src/audio/oal/stream.h

withmorten (Migrated from github.com) commented

The vita sdk doesn't support this, and one of the reasons to do this was to fix freezes when entering cars on the vita port by threading the mp3 seeking code.

madebr (Migrated from github.com) reviewed

src/audio/oal/stream.h

madebr (Migrated from github.com) commented

Thanks for the reply!

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

Please also keep this.

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

Why? Miles doesn't need any OS-level package, nor it's any submodule, why do we need cmake files for it?

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

also if you haven't noticed, all external things that falls under this definition(don't have OS module dependency and not submodule) is now under /src/external, not /vendor. So they're now part of source.

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

e.g. for mingw. This repo only provides binaries for MSVC.

I have added a conan recipe + cmake build script to https://github.com/withmorten/re3mss to rebuild the miles import library.
This is also useful for purists who want to build everything themselve.

e.g. for mingw. This repo only provides binaries for MSVC. I have added a conan recipe + cmake build script to https://github.com/withmorten/re3mss to rebuild the miles import library. This is also useful for purists who want to build everything themselve.

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

also if you haven't noticed, all external things that falls under this definition(don't have OS module dependency and not submodule) is now under /src/external, not /vendor. So they're now part of source.

You can supply vendored versions of third-party libraries to make it easy for developers.
That does not mean that you just break building with system packages or external conan recipes.
If I follow your logic, then we should also remove libsndfile, openal, ogg, ... from conan because it is in the external subfolder.

> also if you haven't noticed, all external things that falls under this definition(don't have OS module dependency and not submodule) is now under /src/external, not /vendor. So they're now _part_ of source. You can supply vendored versions of third-party libraries to make it easy for developers. That does not mean that you just break building with system packages or external conan recipes. If I follow your logic, then we should also remove libsndfile, openal, ogg, ... from conan because it is in the external subfolder.

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

It isn't making it easy, it's making it hard. Having a cmake file for just for one header library doesn't make sense.

Also ogg, opus, opusfile are still in /vendor, because they're used by Linux too, they have OS-level dependencies, and they're submodules.

Whereas libsndfile are now in /src/external, because this folder is only for Windows, and nor it needs any OS-level package for Windows nor it's submodule.

It isn't making it easy, it's making it hard. Having a cmake file for just for one header library doesn't make sense. Also ogg, opus, opusfile are **still** in /vendor, because they're used by Linux too, they have OS-level dependencies, and they're submodules. Whereas libsndfile are now in /src/external, because this folder is only for Windows, and nor it needs any OS-level package for Windows nor it's submodule.

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

I know that we need re3mss repo and I don't want to break it. But I want to remove searching for /vendor/milessdk. I will see what can I do.

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

It isn't making it easy, it's making it hard. Having a cmake file for just for one header library doesn't make sense.

It makes perfect sense in cmake.
It's not "just" one header file. There is also an import library.

> It isn't making it easy, it's making it hard. Having a cmake file for just for one header library doesn't make sense. It makes perfect sense in cmake. It's not "just" one header file. There is also an import library.

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

I know that we need re3mss repo and I don't want to break it. But I want to remove searching for /vendor/milessdk. I will see what can I do.

What do you mean by "searching for /vendor/milessdk"?
The current FIndMilesSDK.cmake looks in MilesSDK_DIR.

> I know that we need re3mss repo and I don't want to break it. But I want to remove searching for /vendor/milessdk. I will see what can I do. What do you mean by "searching for /vendor/milessdk"? The current `FIndMilesSDK.cmake` looks in `MilesSDK_DIR`.

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

It makes perfect sense in cmake.
It's not "just" one header file. There is also an import library.

I don't want that, sorry.

What do you mean by "searching for /vendor/milessdk"?
The current FIndMilesSDK.cmake looks in MilesSDK_DIR.

I mean we don't need find_library, find_path etc. because it will always be in /src/external in current proposal. Because
1- Real SDK and headers will never be officialy released
2- It will never get updated
3- It will no way be submodule due to there is no official repo
4- It will never require a package installed on OS

Thus, I and I think a few more people on the team see no point on naming this as an module.
I'm aware that on MinGW we need to fetch re3mss repo and build it, and I know that I should keep it.
So I'm requesting your help on how can I accomplish this. Do you think just removing find_library and find_path will be enough?

> It makes perfect sense in cmake. It's not "just" one header file. There is also an import library. I don't want that, sorry. > What do you mean by "searching for /vendor/milessdk"? The current FIndMilesSDK.cmake looks in MilesSDK_DIR. I mean we don't need `find_library`, `find_path` etc. because it will always be in /src/external in current proposal. Because 1- Real SDK and headers will never be officialy released 2- It will never get updated 3- It will no way be submodule due to there is no official repo 4- It will never require a package installed on OS Thus, I and I think a few more people on the team see no point on naming this as an module. I'm aware that on MinGW we need to fetch re3mss repo and build it, and I know that I should keep it. So I'm requesting your help on how can I accomplish this. Do you think just removing `find_library` and `find_path` will be enough?

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

Ok, so you want to "hide" miles as an external dependency in the conan recipe and just let the cmake build system handle it itself. Correct?

For building re3 with mingw, we need access to the sources of withmorton's miles sdk.
So you should add it as a git submodule or just copy its files to some vendor subdirectory.

In the cmake build system, you can then include re3mss as a subdirectory, and link to the MilesSDK::MilesSDK target (needs withmorten/re3mss#3).
e.g.

if(${PROJECT}_AUDIO STREQUAL "MSS")
    add_subdirectory("${PROJECT_SOURCE_DIR}/external/re3mss" re3mss)
    target_compile_definitions(${EXECUTABLE} PRIVATE AUDIO_MSS)
    target_link_libraries(${EXECUTABLE} PRIVATE MilesSDK::MilesSDK)
endif()

I will open a small pr at @withmorten 's re3mss repo to make MilesSDK::MilesSDK available + to don't always install its files.

Ok, so you want to "hide" miles as an external dependency in the conan recipe and just let the cmake build system handle it itself. Correct? For building re3 with mingw, we need access to the sources of withmorton's miles sdk. So you should add it as a git submodule or just copy its files to some `vendor` subdirectory. In the cmake build system, you can then include re3mss as a subdirectory, and link to the MilesSDK::MilesSDK target (needs withmorten/re3mss#3). e.g. ```cmake if(${PROJECT}_AUDIO STREQUAL "MSS") add_subdirectory("${PROJECT_SOURCE_DIR}/external/re3mss" re3mss) target_compile_definitions(${EXECUTABLE} PRIVATE AUDIO_MSS) target_link_libraries(${EXECUTABLE} PRIVATE MilesSDK::MilesSDK) endif() ``` I will open a small pr at @withmorten 's re3mss repo to make `MilesSDK::MilesSDK` available + to don't always install its files.

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

I've opened https://github.com/withmorten/re3mss/pull/3

madebr commented (Migrated from github.com)

@erorcun
Would it be possible to don't force push every time and just commit your changes?
It's hard to keep track what you're changing now.
You can squash them into one commit at the end of the ride.

@erorcun Would it be possible to don't force push every time and just commit your changes? It's hard to keep track what you're changing now. You can squash them into one commit at the end of the ride.

👍 1

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

Thank you, but I remember like we already have a code that fetches that repo and compiles it, am i wrong?

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

No, in the conan github ci workflow, a conan package is created from a checked-out re3mss repo.
The re3 conan recipe then uses that re3mss conan package.

The other ci workflows are MSVC specific and use the import library that is available in this repo.

Adding code to cmake to fetch a subproject is possible but would make it not reproducible. Downloading something at build time is not really something nice.

No, in the conan github ci workflow, a conan package is created from a checked-out re3mss repo. The re3 conan recipe then uses that re3mss conan package. The other ci workflows are MSVC specific and use the import library that is available in this repo. Adding code to cmake to [fetch a subproject](https://cmake.org/cmake/help/latest/module/FetchContent.html) is possible but would make it not reproducible. Downloading something at build time is not really something nice.

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

I see, thank you. withmorten just merged the PR, would you like the adapt main repo for it?

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

So would you be okay with re3mss as a git submodule?

withmorten (Migrated from github.com) reviewed

conanfile.py

withmorten (Migrated from github.com) commented

I think that'd be okay, but I'd add a precompiled .lib for MSVC builds at least before that. (The one that's currently in re3/reVC itself)

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

Yup. I'll add the submodule as vendor/milessdk/sources.

Yup. I'll add the submodule as `vendor/milessdk/sources`.

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

And I would appreciate if you can move vendor/milessdk content to src/external/milessdk.
Edit: what is in your mind right now? as you know i don't want to keep vendor/milessdk, so i suggest moving .lib and .h to src/external/milessdk, and use re3mss only when it's needed.

And I would appreciate if you can move vendor/milessdk content to src/external/milessdk. Edit: what is in your mind right now? as you know i don't want to keep vendor/milessdk, so i suggest moving .lib and .h to src/external/milessdk, and use re3mss only when it's needed.

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

Currently, vendor/milessdk has the following tree:

vendor/milessdk
├── include
│   └── mss.h
└── lib
      └── mss32.lib

Note how it contains an import library.

I propose to change it to:

vendor/milessdk
├── include
│   └── mss.h
├── lib
│   └── mss32.lib
└── source
    ├── CMakeLists.txt
    ├── re3mss
    │   ├── mss32.c
    │   ├── mss.h
    │   ├── re3mss.vcxproj
    │   └── re3mss.vcxproj.filters
    ├── re3mss.sln
    └── README.md

Note also that vendor/librw only contains sources, no libraries.
By your logic, vendor/librw should be moved to src/external/librw.

I think you want the following:

keep the import library in vendor/milessdk but remove the header
add re3mss as a submodule in src/external/milessdk and use the header from that location

Currently, `vendor/milessdk` has the following tree: ``` vendor/milessdk ├── include │ └── mss.h └── lib └── mss32.lib ``` Note how it contains an import library. I propose to change it to: ``` vendor/milessdk ├── include │ └── mss.h ├── lib │ └── mss32.lib └── source ├── CMakeLists.txt ├── re3mss │ ├── mss32.c │ ├── mss.h │ ├── re3mss.vcxproj │ └── re3mss.vcxproj.filters ├── re3mss.sln └── README.md ``` Note also that `vendor/librw` only contains sources, no libraries. By your logic, `vendor/librw` should be moved to `src/external/librw`. I think you want the following: - keep the import library in vendor/milessdk but remove the header - add re3mss as a submodule in `src/external/milessdk` and use the header from that location

withmorten (Migrated from github.com) reviewed

conanfile.py

withmorten (Migrated from github.com) commented

Can you check if I need to do more in CMakeLists here? I think "..\include" needs to be added as include dir, but I'm a total cmake noob ...

https://github.com/withmorten/re3mss/tree/test

Can you check if I need to do more in CMakeLists here? I think "..\include" needs to be added as include dir, but I'm a total cmake noob ... https://github.com/withmorten/re3mss/tree/test

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

@withmorten
The cmake script needs a little change:

diff --git a/CMakeLists.txt b/CMakeLists.txt
index b3d7b55..8f7ecd8 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -20,7 +20,7 @@ add_library(re3mss SHARED
 )
 add_library(MilesSDK::MilesSDK ALIAS re3mss)
 target_include_directories(re3mss
-    PUBLIC "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/re3mss>"
+    PUBLIC "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>"
     PUBLIC "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>"
 )

@withmorten The cmake script needs a little change: ```diff diff --git a/CMakeLists.txt b/CMakeLists.txt index b3d7b55..8f7ecd8 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -20,7 +20,7 @@ add_library(re3mss SHARED ) add_library(MilesSDK::MilesSDK ALIAS re3mss) target_include_directories(re3mss - PUBLIC "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/re3mss>" + PUBLIC "$<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>" PUBLIC "$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>" ) ```

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

My proposal is moving vendor/milessdk to src/external/milessdk, and adding vendor/re3mss as a submodule. Me and withmorten both think header should be located in src/external/milessdk.

I don't see it as sources/libraries seperation, I evaluate where it should belong by checking these;

does it need any package on OS?
is it submodule?

since both of them isn't met, I think it belongs to src/external.

My proposal is moving vendor/milessdk to src/external/milessdk, and adding vendor/re3mss as a submodule. Me and withmorten both think header should be located in src/external/milessdk. I don't see it as sources/libraries seperation, I evaluate where it should belong by checking these; - does it need any package on OS? - is it submodule? since both of them isn't met, I think it belongs to src/external.

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

Btw, if the test branch becomes master and if re3 is supposed to use the import library of re3mss.
Then I think re3mss should be kept as a pure git submodule as vendor/re3mss

Btw, if the test branch becomes master and if re3 is supposed to use the import library of re3mss. Then I think re3mss should be kept as a pure git submodule as `vendor/re3mss`

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

So rm -rf vendor/milessdk and everything to src/external/milessdk?

So `rm -rf vendor/milessdk` and everything to `src/external/milessdk`?

erorcun (Migrated from github.com) reviewed

conanfile.py

erorcun (Migrated from github.com) commented

Yes

withmorten (Migrated from github.com) reviewed

conanfile.py

withmorten (Migrated from github.com) commented

Okay that fixed it - I have a question though - cmake currently only builds x64 windows anyway right? since there is no miles library for 64bit for miles 6, this could only be used for x86 windows builds anyway - so the cmakelists in re3mss should also generate 32bit configs.

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

There is no robust way in cmake to force build a 32-bit executable.
With MSVC, this would require a lot of different compiler flags then mingw.
With MSVC's vcvars.bat, the arch is fixed.
If the mingw toolchain is build without multilib support, then this is impossible.

Re3mss's current cmake script creates a mss64 import library, when sizeof(void *) == 8.
The alternative is to fail hard.

Note that the cmake script does also not test on architecture.
It is currently possible to create a 32-bit arm miles import library.

Well, cmake can only build for one compiler/arch/os at a time. This means with a 32-bit compiler, 32-bit executables/libraries will be created. With a 64-bit compiler, 64-bit executables will be generated. There is no robust way in cmake to force build a 32-bit executable. With MSVC, this would require a lot of different compiler flags then mingw. With MSVC's `vcvars.bat`, the arch is fixed. If the mingw toolchain is build without multilib support, then this is impossible. Re3mss's current cmake script creates a `mss64` import library, when `sizeof(void *) == 8`. The alternative is to fail hard. Note that the cmake script does also not test on architecture. It is currently possible to create a 32-bit arm miles import library.

withmorten (Migrated from github.com) reviewed

conanfile.py

withmorten (Migrated from github.com) commented

Hm, so I tried a bit and couldn't get cmake to generate an x86 project, even cmake -A x86 -G "Visual Studio 16 2019" . fails ... even after running vcvars32.bat. Well in any case the .lib can be used for potential 32bit cmake MSVC builds anyway (btw we need to heavily modify the project settings there, currently the RelWithDebInfo builds are basically without optimization, or rather, no inling and cache-unfriendly jumptable in the beginning like a Debug build would have), so I suppose it's still useful for mingw32 builds.

Hm, so I tried a bit and couldn't get cmake to generate an x86 project, even `cmake -A x86 -G "Visual Studio 16 2019" .` fails ... even after running vcvars32.bat. Well in any case the .lib can be used for potential 32bit cmake MSVC builds anyway (btw we need to heavily modify the project settings there, currently the RelWithDebInfo builds are basically without optimization, or rather, no inling and cache-unfriendly jumptable in the beginning like a Debug build would have), so I suppose it's still useful for mingw32 builds.

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

I think you need to do cmake -G "Visual Studio 16 2019" -A Win32.
See https://cmake.org/cmake/help/latest/generator/Visual%20Studio%2016%202019.html#platform-selection

I think you need to do `cmake -G "Visual Studio 16 2019" -A Win32`. See https://cmake.org/cmake/help/latest/generator/Visual%20Studio%2016%202019.html#platform-selection

withmorten (Migrated from github.com) reviewed

conanfile.py

withmorten (Migrated from github.com) commented

Yes, that worked!

madebr (Migrated from github.com) reviewed

conanfile.py

madebr (Migrated from github.com) commented

Check this branch: https://github.com/madebr/re3/tree/milessdk_gitmodule
Its github CI succeeded: https://github.com/madebr/re3/actions?query=branch%3Amilessdk_gitmodule

I used the test branch on the re3mss repo.

Check this branch: https://github.com/madebr/re3/tree/milessdk_gitmodule Its github CI succeeded: https://github.com/madebr/re3/actions?query=branch%3Amilessdk_gitmodule I used the `test` branch on the re3mss repo.

This repo is archived. You cannot comment on pull requests.

No reviewers