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

Move logic out of CMakeLists.txt #2477

Open
wants to merge 13 commits into
base: master
from

Conversation

@ezzieyguywuf
Copy link
Contributor

commented Sep 2, 2019

Thank you for creating a pull request to contribute to FreeCAD! To ease integration, please confirm the following:

  • Branch rebased on latest master git pull --rebase upstream master
  • Unit tests confirmed to pass by running ./bin/FreeCAD --run-test 0
  • Commit message is well-written
  • Commit message includes issue #<id> or fixes #<id> where <id> is the associated MantisBT issue id if one exists

And please remember to update the Wiki with the features added or changed once this PR is merged.
Note: If you don't have wiki access, then please mention your contribution on the 0.19 Changelog Forum Thread.


I am still compiling the code - I'll update this post once I've run all tests successfully.

@luzpaz
Copy link
Contributor

left a comment

Heroic effort here. I found some minor issues that need to be fixed. Thanks!

Edit: typo 😜

# Salome SMESH sources are under src/3rdParty now
if(BUILD_SMESH)
# set the internal smesh version:
# see src/3rdParty/salomonemesh/CMakeLists.txt and commit https://github.com/FreeCAD/FreeCAD/commit/666a3e5 and https://forum.freecadweb.org/viewtopic.php?f=10&t=30838

This comment has been minimized.

Copy link
@luzpaz

luzpaz Sep 2, 2019

Contributor

s/salomonemsh/salomesmesh/

This comment has been minimized.

Copy link
@ezzieyguywuf

ezzieyguywuf Sep 3, 2019

Author Contributor

The "salomonemsh" error is in master. For this initial PR, I wanted to literally copy/paste what is currently in master and simply move it to the cMake sub-directory. In a subsequent future PR I will start going through the actual code/logic and try to clean things like this up.

@@ -0,0 +1,126 @@
macro(FindSalomeMESH)

This comment has been minimized.

Copy link
@luzpaz

luzpaz Sep 2, 2019

Contributor

Should be FindSalomeSMESH (also the file name should be renamed)

This comment has been minimized.

Copy link
@ezzieyguywuf

ezzieyguywuf Sep 3, 2019

Author Contributor

Good catch, I made this change locally but failed to add it to the git repo.

configure_file(SMESH_Version.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/SMESH_Version.h)
endif(BUILD_SMESH)

endmacro(FindSalomeMESH)

This comment has been minimized.

Copy link
@luzpaz

luzpaz Sep 2, 2019

Contributor

s/FindSalomeMESH/FindSalomeSMESH/

@@ -0,0 +1,19 @@
This folder will contain an individual cmake file for each FreeCAD "helper".

A "helper" should be a macro or functio that tries (as much as possible) to adhere to the

This comment has been minimized.

Copy link
@luzpaz

luzpaz Sep 2, 2019

Contributor

s/functio/function/

The idea here is to break up the cmake build system into smaller, more manageable chunks.
This should make maintenance easier, and should also make troubleshooting a bit less
painful. Finally, it should also clean up our top-level CMakeLists.txt file a bit, making
it bit easier for new developpers to jump in and see what's what.

This comment has been minimized.

Copy link
@luzpaz

luzpaz Sep 2, 2019

Contributor

s/developpers/developers/

find_package(Freetype)
if(NOT FREETYPE_FOUND)
message("===============================================================\n"
"FreeType2 not found. Part module will lack of makeWireString().\n"

This comment has been minimized.

Copy link
@luzpaz

luzpaz Sep 2, 2019

Contributor

"FreeType2 not found. Part module will lack functionality of makeWireString().\n"

This comment has been minimized.

Copy link
@ezzieyguywuf

ezzieyguywuf Sep 3, 2019

Author Contributor

Again, this error is currently in master . I would like to leave it as-is for this PR.

@ezzieyguywuf ezzieyguywuf force-pushed the ezzieyguywuf:pull_request/cmake_cleanup branch 2 times, most recently from 836dd65 to a0a9ef6 Sep 3, 2019

@ezzieyguywuf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

I can confirm that all unit tests now pass on my local system.

@ezzieyguywuf ezzieyguywuf force-pushed the ezzieyguywuf:pull_request/cmake_cleanup branch from ba5a249 to 209085d Sep 5, 2019

@ezzieyguywuf ezzieyguywuf force-pushed the ezzieyguywuf:pull_request/cmake_cleanup branch from 209085d to 86ff1d6 Sep 7, 2019

@wwmayer

This comment has been minimized.

ezzieyguywuf added 4 commits Sep 7, 2019
@wwmayer

This comment has been minimized.

Copy link
Contributor

commented Sep 14, 2019

If you look on the travis builds then you will realize that it doesn't work on Windows. It cannot find the Qt5 modules.

@ezzieyguywuf

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2019

@wwmayer I'm aware of the travis issues on Windows. I am working on setting up a build environment in Windows in order to try to duplicate. In the meantime, I've merged in the latest master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.