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

[cmake] Export config properly #262

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

Osyotr
Copy link

@Osyotr Osyotr commented Dec 26, 2023

No description provided.


project(IFCPP)
Copy link
Author

Choose a reason for hiding this comment

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

I've merged this with previous project. There's no reason to do this twice with different names.


ADD_DEFINITIONS(-DIFCQUERY_STATIC_LIB)
ADD_DEFINITIONS(-DIFCQUERY_LIB)
Copy link
Author

Choose a reason for hiding this comment

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

-DIFCQUERY_LIB is required for dynamic builds.

@@ -75,9 +74,14 @@ if (MSVC)
set_source_files_properties(src/ifcpp/IFC4X3/TypeFactory.cpp PROPERTIES COMPILE_FLAGS /bigobj)
endif()

add_library(IfcPlusPlus STATIC ${IFCPP_SOURCE_FILES})
add_library(IfcPlusPlus ${IFCPP_SOURCE_FILES})
Copy link
Author

Choose a reason for hiding this comment

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

Default is STATIC unless BUILD_SHARED_LIBS is ON - no change of behavior here.


TARGET_INCLUDE_DIRECTORIES(IfcPlusPlus
PUBLIC "$<INSTALL_INTERFACE:include;include/ifcpp/IFC4X3/include;include/ifcpp/external>"
Copy link
Author

Choose a reason for hiding this comment

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

This is required for proper INTERFACE_INCLUDE_DIRECTORIES in exported cmake config.

RUNTIME DESTINATION bin
LIBRARY DESTINATION bin
Copy link
Author

Choose a reason for hiding this comment

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

LIBRARY and ARCHIVE target should really be in lib.

@Osyotr
Copy link
Author

Osyotr commented Jan 17, 2024

@ifcquery any feedback?

@ifcquery
Copy link
Owner

Why not keep

project(IfcPlusPlus)

in the cmake file? I don't see why IFCPP is better...

About the lib and bin directory. I have everything in "bin", so everything is in just one place, easy to keep clean or copy stuff to a zip file etc.. And a library file is binary too, so not wrong ;)

@Osyotr
Copy link
Author

Osyotr commented Jan 18, 2024

Why not keep project(IfcPlusPlus) in the cmake file? I don't see why IFCPP is better...

There's no real difference between them except that the exported cmake config name has always been IFCPP. It's possible to keep both project(IfcPlusPlus) and project(IFCPP) but I think it's a code smell.
I'll check if it's possible to keep only project(IfcPlusPlus) but export it as IFCPP to keep backwards compatibility.

About the lib and bin directory. I have everything in "bin", so everything is in just one place, easy to keep clean or copy stuff to a zip file etc..

I find it hard to remember any other project with this installation layout. Pretty much every other library uses lib for libs (.lib, .a and .so). I believe your use cases are covered by cmake --build --target clean (or clean option in VS) and cpack -G ZIP.

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.

2 participants