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.

Sign up for GitHub

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

Fix MODL_Coclustering double size in windows #97

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Fix MODL_Coclustering double size in windows #97

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/Learning/MODL/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
file(GLOB cppfiles ${CMAKE_CURRENT_SOURCE_DIR}/*.cpp)

# MODL executable On Fedora, binaries built with mpi must be suffixed by _mpich (in stored ${MPI_SUFFIX})
# on fedora, binaries built with mpi must ended by _mpich suffix
add_executable(MODL${MPI_SUFFIX} ${cppfiles} MODL.rc)
target_link_libraries(MODL${MPI_SUFFIX} PUBLIC DTForest KMDRRuleLibrary KWLearningProblem)
set_khiops_options(MODL${MPI_SUFFIX})
if(MPI)
target_link_libraries(MODL${MPI_SUFFIX} PUBLIC PLMPI)
endif()

# MODL_SO (shared library)
add_library(MODL_SO SHARED ${cppfiles})
target_link_libraries(MODL_SO PUBLIC DTForest KMDRRuleLibrary KWLearningProblem)
if(MPI)
Expand Down
5 changes: 0 additions & 5 deletions src/Learning/MODL_Coclustering/CMakeLists.txt
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Je ne suis pas d'accord avec la modification des commentaires. Il faut que tu arretes de te focaliser sur les target. TOUT est target dans cmake. Par exemple pour, add_binary(TOTO) et add_executable(TITI) dans le commentaire, on ne va pas mettre qu'on ajoute la target TOTO et la target TOTI mais bien l'executable TOTO et la bibliothèque TITI car c'est ce qu'oin souhaite faire. Et que cmake passe par des targets, c'est secondaire et ça n'apporte absolument rien à la comprehension du fichier CMakeList qu'on est en train de commenter.
Ici en l'occurence on parle du binaire MODL_Coclustering ou du nom de ce binaire. D'ailleurs pour fedora on n'a pas du tout besoin de renommer la target mais bien le nom du binaire !!!
Mon commentaire n'était peut-être pas précis car je faisis un abus de le langage, en effet il ne faut pas renommer le binaire mais le nom du binaire, soit ! On aurait donc on fedora, binaries names built with mpi must be suffixed by _mpich. Je pense qu'il est important de commencer le commentairte par on fedora car ça ne concerne que fedora...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I will be more precise for each target type (binary or library). I would keep separated the code for each of them to avoid bugs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was also a formatting error of cmake-format I didn't mean to put On Fedora after the title of the section.

Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,8 @@ file(GLOB cppfiles ${CMAKE_CURRENT_SOURCE_DIR}/*.cpp)
# on fedora, binaries built with mpi must ended by _mpich suffix
add_executable(MODL_Coclustering${MPI_SUFFIX} ${cppfiles} MODL_Coclustering.rc)
set_khiops_options(MODL_Coclustering${MPI_SUFFIX})

target_link_libraries(MODL_Coclustering${MPI_SUFFIX} PUBLIC KMDRRuleLibrary KWLearningProblem)

if(MSVC)
target_link_options(MODL_Coclustering${MPI_SUFFIX} PUBLIC "/INCREMENTAL")
endif()

add_library(MODL_Coclustering_SO SHARED ${cppfiles})
target_link_libraries(MODL_Coclustering_SO PUBLIC KMDRRuleLibrary KWLearningProblem)
set_target_properties(
Expand Down
Loading