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

Switching to CMake; #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

raue
Copy link

@raue raue commented Feb 12, 2024

switching to cmake in order to focus on supported OSs

CMakeLists.txt Outdated
message(FATAL_ERROR "${CMAKE_SYSTEM_NAME} OS detected - currently not supported - EXIT")
endif()

include_directories(nav)

Choose a reason for hiding this comment

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

I suggest using target_include_directories() instead.

Copy link
Author

Choose a reason for hiding this comment

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

ok.
would lead to something like this, or?
cmake_minimum_required(VERSION 3.19.0)

project(NCOM2CSV)

if(${CMAKE_SYSTEM_NAME} MATCHES Darwin)
message("Apple OS (Darwin) detected")
elseif(${CMAKE_SYSTEM_NAME} MATCHES Linux)
message("Linux OS detected")
else()
message(FATAL_ERROR "${CMAKE_SYSTEM_NAME} OS detected - currently not supported - EXIT")
endif()

set(SOURCES nav/NComRxC.c example/NcomToCsv.c)
add_executable(ncom2csv ${SOURCES})
target_include_directories(ncom2csv PRIVATE nav)

Choose a reason for hiding this comment

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

Yes, looks like the way to go.

endif()

include_directories(nav)
set(SOURCES nav/NComRxC.c example/NcomToCsv.c)

Choose a reason for hiding this comment

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

I suggest listing header files as well for the sake of completeness and explicitness. Despite this is not really necessary to build the code, some IDEs benefit from that.

Copy link
Author

Choose a reason for hiding this comment

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

well, pointing to a directory instead of single files would keep the script - slightly - leaner and easier to maintain. So I'd propose just to mention the include directory by using the suggested target_include_directories().

Choose a reason for hiding this comment

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

Any of the suggested approaches will allow to build the code. This is more about convenience with regards to personal habbits and tools used by active contributors. So, they are the ones to decide.

Basically, the trade-off is whether one wants less writing CMakeLists or have more robust setup with regards to tooling like IDEs and Git in terms of active switching between branches.

Up to you.

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