[merged] Cleanup Top-Level CMakeLists.txt

Merged, abandoned or rejected pull requests are moved here to clear the main Pull Requests forum.
ezzieyguywuf
Posts: 637
Joined: Tue May 19, 2015 1:11 am

[merged] Cleanup Top-Level CMakeLists.txt

Postby ezzieyguywuf » Tue Sep 03, 2019 11:21 pm

Per the discussion here, I have created a pull-request which moves the majority of the logic out of our top-level CMakeLists.txt and instead segregates them into different macros.

The pull-request can be seen here, PR2477. I have also created a README.md which tries to outline the intent of splitting out the logic.

In general, I am open to any critique on the system used. I have simply created macros with practical names.

I elected to put each into its own file because I've run into issues in the past where large changes to CMakeLists.txt caused merge conflicts in git. Hopefully giving each macro its own file can reduce these conflicts.

I have two main areas that I would say I am "concerned" about, or perhaps put better that I am seeking input on:
  1. The use of file(GLOB...) and foreach(..) in cMake/FreeCAD_Helpers/CMakeLists.txt
  2. The seemingly random naming conventions used for the various files in cMake/FreeCAD_Helpers
Here are the thought processes behind each of these decisions:
  1. This choice broke down to one of two:
    1. Use the file(GLOB...) technique, which should allow any future developer to simply drop their file in cMake/FreeCAD_Helpers and go about their business
    2. Individually add each new file to cMake/FreeCAD_Helpers/CMakeLists.txt
    I opted for the first of these choices because it seems like a newish user of CMake might not necessarily catch the need to add the file to cMake/FreeCAD_Helpers/CMakeLists.txt and that this could lead to frustration and a lack of maintaining this structure.
  2. The filenames were chosen to match the macro names in order to remain descriptive. However, I have considered prepending each filename with an ordering prefix, i.e. "10_CompilerChecksAndSetups.cmake", "15_ConfigureCMakeVariables.cmake" etc... This has the effect of allowing us to order the filenames in cMake/FreeCAD_Helpers in the same order as they appear in CMakeLists.txt. However, I could think of no practical usefulness for this feature aside from my own OCD as well as making it somewhat easier to see the parallels between the contents of cMake/FreeCAD_Helpers and CMakeLists.txt.
Last edited by ezzieyguywuf on Sun Oct 06, 2019 6:51 pm, edited 1 time in total.
ezzieyguywuf
Posts: 637
Joined: Tue May 19, 2015 1:11 am

Re: Cleanup Top-Level CMakeLists.txt

Postby ezzieyguywuf » Thu Sep 19, 2019 5:32 pm

This pull request is now passing the travis builds.
wmayer
Site Admin
Posts: 15129
Joined: Thu Feb 19, 2009 10:32 am

Re: Cleanup Top-Level CMakeLists.txt

Postby wmayer » Thu Sep 19, 2019 5:33 pm

OK, I will recheck it at the weekend.
User avatar
Kunda1
Posts: 6223
Joined: Thu Jan 05, 2017 9:03 pm

Re: Cleanup Top-Level CMakeLists.txt

Postby Kunda1 » Thu Sep 19, 2019 5:54 pm

maybe we should give a heads up to the downstream packagers before we merge this?
Want to contribute back to FC? Checkout:
#lowhangingfruit | Use the Source, Luke. | How to Help FreeCAD | How to report FC bugs and features
wmayer
Site Admin
Posts: 15129
Joined: Thu Feb 19, 2009 10:32 am

Re: Cleanup Top-Level CMakeLists.txt

Postby wmayer » Thu Sep 19, 2019 6:08 pm

Kunda1 wrote:
Thu Sep 19, 2019 5:54 pm
maybe we should give a heads up to the downstream packagers before we merge this?
They shouldn't be affected as this is supposed to be an internal change.
User avatar
Kunda1
Posts: 6223
Joined: Thu Jan 05, 2017 9:03 pm

Re: Cleanup Top-Level CMakeLists.txt

Postby Kunda1 » Thu Sep 19, 2019 6:40 pm

wmayer wrote:
Thu Sep 19, 2019 6:08 pm
They shouldn't be affected as this is supposed to be an internal change.
OK, maybe I'm confusing PRs. I was thinking that if downstream packagers are patching against CMakeLists.txt then there would be an issue...
Want to contribute back to FC? Checkout:
#lowhangingfruit | Use the Source, Luke. | How to Help FreeCAD | How to report FC bugs and features
ezzieyguywuf
Posts: 637
Joined: Tue May 19, 2015 1:11 am

Re: Cleanup Top-Level CMakeLists.txt

Postby ezzieyguywuf » Thu Sep 19, 2019 8:08 pm

Kunda1 wrote:
Thu Sep 19, 2019 5:54 pm
maybe we should give a heads up to the downstream packagers before we merge this?
wmayer wrote:
Thu Sep 19, 2019 6:08 pm
They shouldn't be affected as this is supposed to be an internal change.
@wmayer is correct, this does not affect the build process at all. This simply relocates code that is currently in top-level CMakeLists.txt and moves it to cMake/FreeCAD_Helpers sub-directory.