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

Refactor CMake Build System #150

Closed
david-cattermole opened this issue Sep 11, 2020 · 14 comments
Closed

Refactor CMake Build System #150

david-cattermole opened this issue Sep 11, 2020 · 14 comments
Assignees
Labels
builld system Related to the Build system for the project.
Milestone

Comments

@david-cattermole
Copy link
Owner

Problem

The current use of CMake as the build generator works, but is far from ideal.
We need to do a refactor of the CMake build system code.

Problems to be fixed:

  • The CMakeLists.txt files are not organised in a hierarchical way, with separation of concerns.

  • There is a lot of duplication of code, which should be condensed into re-useable macros/functions.

  • "Modern" CMake practices must be used, avoiding global changes to include directory paths and instead using "targets".

Software Versions

  • mmSolver version: Latest

  • Maya version: All

  • Operating System (OS): All (including Mac)

@david-cattermole david-cattermole added the builld system Related to the Build system for the project. label Sep 11, 2020
@david-cattermole david-cattermole self-assigned this Sep 11, 2020
@david-cattermole david-cattermole added this to the v0.3.7 milestone Sep 16, 2020
@david-cattermole david-cattermole modified the milestones: v0.3.7, v0.4.0 Oct 10, 2020
david-cattermole pushed a commit that referenced this issue Oct 14, 2020
Disable levmar for short term - with levmar configured but
not installed, the build fails.

Issues #146 and #150.
david-cattermole pushed a commit that referenced this issue Oct 16, 2020
@ktonegawa
Copy link
Contributor

Hey @david-cattermole

So as per our previous conversation, I installed the Cross Tools Command Prompt for VS2015, I guess I just never installed the C++ components for whatever reason.

Anyways, I've noticed that when trying to run the included batch file for a quick build, this fails because on my system i have MinGW installed.

If you remember, previously I had avoided this issue by first generating the VS solution file and manually removing C:\MinGW from the Include Directories, but I felt for future users potentially wanting to build this from source in Windows, it would be beneficial include sort of a "Ignore directory" option somewhere to avoid this error for a smoother build. And since MinGW I feel is pretty commonly used to build a lot of other software.

So I pushed this little modification that avoids this issue here: ktonegawa@7be8448

I thought perhaps this could help sort of partially solve issues described in this ticket. What do you think?

I would also like to address the issue with #149 but I don't really know how to do deal with that with Windows, as I am not sure where the icons/CMakeList.txt is really being triggered for it...

@david-cattermole
Copy link
Owner Author

Hey @ktonegawa,

That's not a bad idea to try and avoid using MinGW by default. Ignoring predefined directories feels like a hack to me, but in the short-term I'm fine to include that until I come up with a better way to only use Visual Studio includes.

Your commit ktonegawa/mayaMatchMoveSolver@7be8448 seems ok (I haven't tested it myself yet), however it needs to use the "develop" branch, which is primed for v0.4.x release. As bad as the current build system is, I don't want to change it completely right now and instead only "maintain" the current build system until v0.4.x is released. I am making all sorts of build-system breaking changes in that branch with the aim of making everything cleaner and simpler to build on all 3 OSs. I have just merged in the latest changes from #146 (to support MacOS) which includes a lot of CMake changes.

Also, if you add this change, can you also add it for Maya 2016, 2017 and 2018? I noticed your current commit is only for Maya 2019.
(Note: I'm not planning to build for Maya 2016 and 2017 in the v0.4.x release, but it might still be good to keep the files around for now.)

I would also like to address the issue with #149 but I don't really know how to do deal with that with Windows, as I am not sure where the icons/CMakeList.txt is really being triggered for it...

To remove Qt as a build dependency (#149) I was planning to use the Maya devkit, which contains a pre-compiled rcc binary for each platform which will have the exact version used in Maya and will therefore be compatible. I have included support for using the Maya devkit in #146, because getting the correct Qt version required on MacOS was difficult - however I think that has been only tested on MacOS. Perhaps you could try building the latest "develop" branch on Windows?

Perhaps an easier, but tedious Qt-related issue is #156. The details can be found here. Unfortunately this will require changing every single Python Qt import statement to point to the new import path.

David

@david-cattermole
Copy link
Owner Author

P.S.

but I don't really know how to do deal with that with Windows, as I am not sure where the icons/CMakeList.txt is really being triggered for it...

icons/CMakeList.txt is added as a subdirectory here:

add_subdirectory(icons)

The Maya Devkit is added as a CMake option here:

-DDEVKIT_LOCATION=${DEVKIT_LOCATION} \

... although after reading the CMake source code I am not sure how the Maya devkit actually makes it easier for the correct rcc executable to used or found. Here is where rcc is run via CMake:

COMMAND rcc -binary resources.qrc -o resources.rcc

@ktonegawa
Copy link
Contributor

That's not a bad idea to try and avoid using MinGW by default. Ignoring predefined directories feels like a hack to me, but in the short-term I'm fine to include that until I come up with a better way to only use Visual Studio includes.

Hey @david-cattermole, well I'm glad I didn't make a PR yet. And yeah I think I can see what you mean by this feeling like a hack. Since there are many other compilers out there like MinGW, I would like to more just some how start the includes of this to just default to vanilla Visual Studio settings, but I couldn't figure out how to do that with CMake. Do you think I should consult with the Stackoverflow hivemind about this?

Your commit [ktonegawa/mayaMatchMoveSolver@7be8448] ktonegawa@7be8448) seems ok (I haven't tested it myself yet), however it needs to use the "develop" branch, which is primed for v0.4.x release. As bad as the current build system is, I don't want to change it completely right now and instead only "maintain" the current build system until v0.4.x is released. I am making all sorts of build-system breaking changes in that branch with the aim of making everything cleaner and simpler to build on all 3 OSs. I have just merged in the latest changes from #146 (to support MacOS) which includes a lot of CMake changes.

Regarding the "develop" branch, I thought I had made my new branch off of develop... Am I doing something wrong with my forked repo?

What I did after running git fetch upstream;git pull, I initially attempted to checkout develop via git checkout develop but it would error out saying there are "two branches starting with the name develop", so I had to checkout via git checkout origin/develop and that would just give me this ID as a branch name, and then I did git checkout -b issue150-windows. But I would not be surprised if this actually didn't work.

Also, if you add this change, can you also add it for Maya 2016, 2017 and 2018? I noticed your current commit is only for Maya 2019.

Yup for sure, again I only modified the batchfile for 2019 as more of a proof of concept.

With the rcc issue: what I am wanting to be careful on is because the icons/CMakeList.txt seems OS independent, I was more wondering where this was being initiated, but I guess that is

add_subdirectory(icons)

Then the next question is at least for Windows, where to implement a condition where if rcc is not valid, how then to replace that with the rcc.exe file via Maya's DevKit you are referring to (the one directly under Maya's /bin directory).

@david-cattermole
Copy link
Owner Author

Hello @ktonegawa,

Regarding the "develop" branch, I thought I had made my new branch off of develop... Am I doing something wrong with my forked repo?

Actually, I didn't check very well, it looks like you are using develop.
I just made some changes 11 hours, can you merge in my latest changes so you're up to date?

Merging the latest changes should look like this (without $):

$ git pull upstream/develop

Or...

$ git fetch
$ git merge upstream/develop

With the rcc issue: what I am wanting to be careful on is because the icons/CMakeList.txt seems OS independent, I was more wondering where this was being initiated, but I guess that is

The current CMake file should work on all platforms, CMake is smart enough to add the .exe for Windows automatically, same goes for file paths. The forward slashes (/) and are converted to backslash (\) automatically by CMake on Windows. (Internally CMake uses forward slash everywhere).

Then the next question is at least for Windows, where to implement a condition where if rcc is not valid, how then to replace that with the rcc.exe file via Maya's DevKit you are referring to (the one directly under Maya's /bin directory).

I think the best way to handle this is to find the rcc executable using a find_program command in CMake, like this:

find_program(MAYA_PYTHON_EXECUTABLE

If rcc cannot be found then mayapy and maya will be not be found either and there is something seriously wrong - nothing we can do about that. In the worst case scenario the user can turn off building the icons with the BUILD_ICONS flag to CMake.

Hopefully we can add some default relative paths from the $MAYA_LOCATION to get the rcc that is installed with Maya.
On Windows it appears to be stored here: C:\Program Files\Autodesk\Maya2018\bin\rcc.exe ($MAYA_LOCATION/bin/rcc). I suspect Linux will be the same as Windows. I can handle the (probably) special case of MacOS if you'd like.

@david-cattermole
Copy link
Owner Author

Hello @ktonegawa,

And yeah I think I can see what you mean by this feeling like a hack. Since there are many other compilers out there like MinGW, I would like to more just some how start the includes of this to just default to vanilla Visual Studio settings, but I couldn't figure out how to do that with CMake.

I did a bunch of reading (Googling) about this. I think this is a special case for Windows, and since we know Maya expects a specific compiler and using MinGW will not work I think it's a good idea to use CMAKE_IGNORE_PATH.
https://cmake.org/cmake/help/latest/variable/CMAKE_IGNORE_PATH.html#variable:CMAKE_IGNORE_PATH

CMAKE_PREFIX_PATH could also be used to prefer a specific path, but I think it's complicated for all the different Visual Studio paths (I'm not sure which one to use), so I think your idea of CMAKE_IGNORE_PATH is best.
https://cmake.org/cmake/help/latest/variable/CMAKE_PREFIX_PATH.html#variable:CMAKE_PREFIX_PATH

Also, since we add this to the .bat file it is easy to configure or remove if we need to.

@ktonegawa
Copy link
Contributor

Hi @david-cattermole , just as a reference I put this together in an attempt to address the rcc not found issue: ktonegawa@8f14a42

The adjustments to the 2016-2018 build batch files are still on my to do list...

@david-cattermole
Copy link
Owner Author

Ok, that's good to know, thanks.
I have added a few comments, I hope you don't mind.

@ktonegawa
Copy link
Contributor

Thank you @david-cattermole! Responded to them for further clarification.

ktonegawa added a commit to ktonegawa/mayaMatchMoveSolver that referenced this issue Jan 9, 2021
david-cattermole added a commit that referenced this issue Jan 9, 2021
Issue 150 For Windows OS - An attempt to address #150 on the Windows end.

These changes attempts to address two things:

1) The issue of Visual Studio defaulting to source MinGW as its main C++ compiler

2) The issue of rcc not being found when coming to the part where it 
builds the UI/Documentation sections (I believe these are the relevant parts)

Added a scripts/build_mmSolver_windows64_maya2016_extension2.bat so that users who may be using Maya 2016 Extenstion 2 (2016.5).
@david-cattermole
Copy link
Owner Author

BTW, I have been using the ExternalProject module in CMake 3.0 to build projects. I think this will make most, if not all of the Python scripts I'm using to download and build external dependencies completely unneeded.

For this benefit I think it makes absolute sense to use CMake 3.0. as a minimum supported CMake version.
This might mean that folks on CentOS 7.x will need to install an external package (because CentOS 7.x only seems to natively support CMake 2.8.12), and I hope that won't be too off-putting.

Here is an example setup I have been writing for OpenCompGraphMaya:
https://github.com/david-cattermole/OpenCompGraphMaya/tree/master/thirdparty
I will port this build system over to mmSolver in time, once OpenCompGraphMaya is ready for deploy and I'm happy that the build system fits everyone's needs.

@ktonegawa
Copy link
Contributor

Oh that's interesting. I guess ExternalProject allows you to directly checkout/pull a git repository?

Still though, I am not sure about making CMake 3.0 as a minimum supported version. Some studios which may be utilizing this plugin may have strict policies on installed versions of software (and to maintain their build systems like you mentioned to me before). Might be better to have an option to utilize either or for the time being...

@david-cattermole
Copy link
Owner Author

Oh that's interesting. I guess ExternalProject allows you to directly checkout/pull a git repository?

Yeah, we can clone a git repo, or download source code directly, then patch it if needed, and finally we can then launch the CMake build on it, in a different directory. It's got a lot of functionality.

Still though, I am not sure about making CMake 3.0 as a minimum supported version. Some studios which may be utilizing this plugin may have strict policies on installed versions of software (and to maintain their build systems like you mentioned to me before). Might be better to have an option to utilize either or for the time being...

CMake 2.8.12 was released October 8, 2013, I really hope we can move on from 2013, considering there is no binary compatibility issues (unlike changing compilers), and the VFX Reference Platform does not specify which version of CMake to use.

I double checked the version and it seems ExternalProject is available in CMake 2.8.12 (but it's lacking some features compared to the newest version), so it might not be a big problem. 2.8.12 is the "first version" to contain "Modern CMake" features.

I'll discuss with some people offline and see if CMake 2.8.12 is a sticking point for them, I really do want to use something newer for mmSolver v0.4.x.

P.S. If you ever have a spare 3.5 hours, and you don't want to re-watch a few The Lord of the Rings movies, these talks are informative:
C++Now 2017: Daniel Pfeifer “Effective CMake"
CppCon 2017: Mathieu Ropert “Using Modern CMake Patterns to Enforce a Good Modular Design”
Deep CMake for Library Authors - Craig Scott - CppCon 2019

@david-cattermole
Copy link
Owner Author

Someone recently pointed out to me that OpenEXR requires at least CMake v3.10 to build, and prefers v0.3.12, so I think it's safe to say we should be able to use at least v3.10.

david-cattermole added a commit that referenced this issue Nov 27, 2021
This includes building gflags, glog and eigen.

We also include build recipies for CMinpack and LevMar, so we
can deprecate the older methods of building and keep a single
cross-platform CMake approach.

Issues #144 #150 #174

Change-Id: Id4a8fb7c29be44f58101fdf555444078da91fed6
@david-cattermole
Copy link
Owner Author

The CMake build scripts have been improved a huge amount.
The build scripts use "Modern CMake" techniques, and avoid in-source building where possible.

I'm going to consider this issue finished. Any new issues or features needed will be addressed in a new issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builld system Related to the Build system for the project.
Projects
None yet
Development

No branches or pull requests

2 participants