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

Refactored CMake build scripts #2189

Open
wants to merge 20 commits into
base: master
Choose a base branch
from

Conversation

bradc6
Copy link

@bradc6 bradc6 commented Mar 18, 2020

Refactored the cmake files to be easier to read, the main could use more work though. Thoughts?

@bradc6 bradc6 changed the title Refactored Refactored CMake build scripts Mar 18, 2020
@gonetz
Copy link
Owner

gonetz commented Mar 19, 2020

I'm not a cmake expert and can't say is everything done correctly or not. New version definitely looks more clear. If it works properly I'm ready to merge it. I put this PR into cmake_cleanup branch. @loganmc10, @fzurita could you test that branch?

@loganmc10
Copy link
Contributor

I'll test in a bit, isn't 3.15.5 a little high for a minimum version though? The current LTS version of Ubuntu comes with 3.10

@bradc6
Copy link
Author

bradc6 commented Mar 20, 2020

We need 3.15.5 for target_link_directories

@loganmc10
Copy link
Contributor

We need 3.15.5 for target_link_directories

I think 3.15.5 is too high, it's going to stop working for anyone with a Raspberry Pi I would guess

https://cmake.org/cmake/help/git-stage/command/target_link_directories.html

This command is rarely necessary and should be avoided where there are other choices. Prefer to pass full absolute paths to libraries where possible, since this ensures the correct library will always be linked.

@dankcushions
Copy link
Contributor

current raspbian's cmake is 3.13.4, and raspbian is probably more up to date than some of GLideN64's targets.

agreed that this cannot be a requirement.

@bradc6
Copy link
Author

bradc6 commented Mar 22, 2020

What are the libs that need to be linked with on raspbian? (aka the fill filepath) With that we can make them an import target and drop the version down.

@bradc6
Copy link
Author

bradc6 commented Mar 22, 2020

Are we cool with requiring Cmake 3.13?

@loganmc10
Copy link
Contributor

Are we cool with requiring Cmake 3.13?

The current LTS version of Ubuntu (supported until 2023 I think) has cmake 3.10. if we require higher than that, I think it will stop working for a good number of users

@fzurita
Copy link
Contributor

fzurita commented Mar 22, 2020

Android only has CMake only up to 3.10 as well.

@gonetz
Copy link
Owner

gonetz commented Mar 22, 2020

Are we cool with requiring Cmake 3.13?

Travis successfully built your PR for Linux and Mac.
In this respect we are cool.

It would be good to setup CI build for RPI and Android too, I don't know how.

The current LTS version of Ubuntu (supported until 2023 I think) has cmake 3.10. if we require higher than that, I think it will stop working for a good number of users

Current Ubuntu LTS is 18.04.4 (Bionic Beaver), is not it? This PR successfully built with it, but it is probably because it uses updated repo for cmake, commit a31c711

Android only has CMake only up to 3.10 as well

Then we are stuck with 3.10

Bradley, can you cleanup cmake files using 3.10 version?

@bradc6
Copy link
Author

bradc6 commented Apr 11, 2020

@gonetz What platforms are hard vs soft supported?
@fzurita How is your android environment setup?
@fzurita Can you make a CI for it in order to test against?
@dankcushions or @loganmc10 How is your raspbian environment setup?
@dankcushions or @loganmc10 Can you make a CI for it in order to test against?

@dankcushions
Copy link
Contributor

Raspbian is just a prebuilt distro for raspberry pis. it's based on debian buster: https://www.raspberrypi.org/downloads/raspbian/. i won't make a CI but happy to test this PR at my end once the changes are done.

@bradc6 bradc6 force-pushed the feature/CleanupCmake branch from 3ed59a9 to 3e8b50b Compare April 12, 2020 19:27
@bradc6
Copy link
Author

bradc6 commented Apr 12, 2020

The branch now builds on raspbian

@fzurita
Copy link
Contributor

fzurita commented Apr 12, 2020

@bradc6 if you make a pull request here it will automatically try to build it:

https://github.com/mupen64plus-ae/mupen64plus-ae/pulls

You can try your updates on this file: https://github.com/mupen64plus-ae/mupen64plus-ae/blob/master/mupen64plus-video-gliden64/upstream/src/CMakeLists.txt

@bradc6
Copy link
Author

bradc6 commented Apr 13, 2020

@fzurita Thank you for the link, I'll give that a try and see if I can add android CI directly onto this repo

@bradc6
Copy link
Author

bradc6 commented Apr 13, 2020

If I get windows also building in cmake is there any reason to keep the msvc project either? That way there is nothing to "fall out of sync" against

@gonetz
Copy link
Owner

gonetz commented Apr 14, 2020

If I get windows also building in cmake is there any reason to keep the msvc project either?

If cmake will generate completely equivalent msvc project, no.
Currently msvc project contains configurations for zilmar-spec (which includes build of QT-based GUI), mupen64plus, both in debug and release, 32bit and 64bit, with post-build steps.
If cmake can build such a project, great.
It would also be great to automatically update the project when new files added.

@gonetz gonetz force-pushed the master branch 2 times, most recently from 28c4deb to 3fe5f50 Compare April 25, 2020 13:46
@bradc6 bradc6 force-pushed the feature/CleanupCmake branch from 3e8b50b to 5e6e108 Compare May 24, 2020 03:37
@bradc6
Copy link
Author

bradc6 commented May 24, 2020

@gonetz It can be done
@fzurita Made a test @ mupen64plus-ae/mupen64plus-ae#824 for testing

@bradc6
Copy link
Author

bradc6 commented Jun 1, 2020

Bump?

@gonetz
Copy link
Owner

gonetz commented Jun 1, 2020

Hi Bradley,

I just tried to build your PR with Android Studio:

/home/sergey/WS/mupen64plus-ae/mupen64plus-video-gliden64/upstream/src/CMakeLists.txt : C/C++ debug|x86 : CMake Error at /home/sergey/WS/mupen64plus-ae/mupen64plus-video-gliden64/upstream/src/CMakeLists.txt:1 (cmake_minimum_required):
  CMake 3.13.4 or higher is required.  You are running version 3.10.2
Affected Modules: mupen64plus-video-gliden64

Android SDK uses CMake 3.10.2.4988404. I have Ubuntu 18.4 on my PC. It also uses 3.10.2
It is not a problem to install a recent version, but it is an additional effort for Ubuntu users. As I see, Ubuntu 20.04 uses CMake 3.16.3 but it was released just a month ago. Anyway, we can't break compatibility with Android Studio. I have no idea when they are going to update their internal cmake version. So, we are stuck with 3.10.2 for now.

@gonetz
Copy link
Owner

gonetz commented Jun 1, 2020

Is it possible to check CMake version first and fall back to current 3.10 variant if CMake 3.13.4 or higher is not available?

@bradc6
Copy link
Author

bradc6 commented Jun 15, 2020

@gonetz Updated to 3.10. Could you build again?

@gonetz
Copy link
Owner

gonetz commented Jun 15, 2020

Ubuntu 18.04, CMakeOutput.log attached
CMakeOutput.log
:

~/WS/GLideN64/projects/cmake$ cmake -DCMAKE_BUILD_TYPE=Debug -DVEC4_OPT=On -DUSE_SYSTEM_LIBS=On -DMUPENPLUSAPI=On ../../src/
-- The C compiler identification is GNU 7.5.0
-- The CXX compiler identification is GNU 7.5.0
-- Check for working C compiler: /usr/bin/cc
-- Check for working C compiler: /usr/bin/cc -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Detecting C compile features
-- Detecting C compile features - done
-- Check for working CXX compiler: /usr/bin/c++
-- Check for working CXX compiler: /usr/bin/c++ -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
You have called ADD_LIBRARY for library GLideN64 without any source files. This typically indicates a problem with your CMakeLists.txt file
-- Found ZLIB: /usr/lib/x86_64-linux-gnu/libz.so (found version "1.2.11")
-- Found PNG: /usr/lib/x86_64-linux-gnu/libpng.so (found version "1.6.34")
You have called ADD_LIBRARY for library osal without any source files. This typically indicates a problem with your CMakeLists.txt file
CMake Error at osal/CMakeLists.txt:26 (target_link_directories):
Unknown CMake command "target_link_directories".

-- Configuring incomplete, errors occurred!
See also "/home/sergey/WS/GLideN64/projects/cmake/CMakeFiles/CMakeOutput.log".

@gonetz
Copy link
Owner

gonetz commented Jun 15, 2020

Android Studio:

/home/sergey/WS/mupen64plus-ae/mupen64plus-video-gliden64/upstream/src/CMakeLists.txt : C/C++ debug|x86 : CMake Error at /home/sergey/WS/mupen64plus-ae/mupen64plus-video-gliden64/upstream/src/osal/CMakeLists.txt:26 (target_link_directories):
Unknown CMake command "target_link_directories".
Affected Modules: mupen64plus-video-gliden64

… does leak context but there is nothing we can really do about that at the moment
@gonetz
Copy link
Owner

gonetz commented Jul 2, 2020

cmake -DCMAKE_BUILD_TYPE=Debug -DVEC4_OPT=On -DUSE_SYSTEM_LIBS=On -DMUPENPLUSAPI=On ../../src/
You have called ADD_LIBRARY for library GLideN64 without any source files. This typically indicates a problem with your CMakeLists.txt file
You have called ADD_LIBRARY for library osal without any source files. This typically indicates a problem with your CMakeLists.txt file
CMake Error at osal/CMakeLists.txt:26 (target_link_directories):
Unknown CMake command "target_link_directories".

@bradc6
Copy link
Author

bradc6 commented Jul 2, 2020

a missed some, one moment

@fzurita
Copy link
Contributor

fzurita commented May 7, 2021

FYI, Android Studio now supports CMake 3.18

@gonetz
Copy link
Owner

gonetz commented May 15, 2021

FYI, Android Studio now supports CMake 3.18

We should be good now. @bradc6 could you re-test your work on Windows and Linux(es)?

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.

5 participants