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: Partial fix for FetchContent #36

Merged
merged 3 commits into from
Jun 14, 2023
Merged

fix: Partial fix for FetchContent #36

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3d186fb
Deprecated `make_directory`
LecrisUT Jun 12, 2023
93d9872
Partial fix of FetchContent
LecrisUT Jun 12, 2023
bbd0b7a
Remove unnecessary settings
LecrisUT Jun 12, 2023
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
24 changes: 12 additions & 12 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,10 @@ project(fdict
DESCRIPTION "Fortran dictionary for arbitrary data-types"
)

if (NOT DEFINED CMAKE_Fortran_MODULE_DIRECTORY)
set(CMAKE_Fortran_MODULE_DIRECTORY ${CMAKE_CURRENT_BINARY_DIR}/fortran_modules)
Copy link
Owner

Choose a reason for hiding this comment

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

add quotation, to ensure paths with spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have tested locally and the quotation does not appear to be necessary. In general I would avoid quotations where unnecessary (in this case the variable CMAKE_CURRENT_BINARY_DIR is already sanitized to be a string) because sometimes the quotations are taken to be literal (as you've seen here in the generator expression). My rule of thumb is to sanitize everything at the set(), option() level rather than where the variables are used.

Copy link
Owner

Choose a reason for hiding this comment

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

I see, this is something where CMake is horrible, it is hard to know why some of the expansions works seamlessly, and others are problematic... Hence my guarding was just to put quotation everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fully agree. This is a relic of the original cmake design, and although there are rumors that the lead devs are working on a more robust, non text-based interface for camke 4, these are here to stay :(.

I was doing the same thing until I had various things break down because of the quotation. I realized though that most cmake commands are designed to work on variables (e.g. the if() command), so I work on the assumption that most commands are designed and tested to work for unquoted formats, and just use the quotation when it is needed to enforce it.

endif ()

# Project installation follows GNU installation directory convention
include(GNUInstallDirs)

Expand Down Expand Up @@ -44,8 +48,8 @@ list(APPEND FYPPFLAGS
)

# Add project options
include("${CMAKE_CURRENT_SOURCE_DIR}/cmake/fdictOptions.cmake")
include("${CMAKE_CURRENT_SOURCE_DIR}/cmake/fdictFyppify.cmake")
include(cmake/fdictOptions.cmake)
include(cmake/fdictFyppify.cmake)

# create library.
add_subdirectory(src)
Expand All @@ -54,11 +58,10 @@ add_subdirectory(src)
# Now for installing stuff.

# Generate the version include file
configure_file(
"${CMAKE_CURRENT_SOURCE_DIR}/fdict.inc.in"
"${CMAKE_CURRENT_BINARY_DIR}/fdict.inc"
@ONLY
)
# TODO: This file should not be necessary
# This file is already parsed in the preprocessor
# Instead move this information into fortran functions to extract the version
configure_file(fdict.inc.in fdict.inc)
install(
FILES
"${CMAKE_CURRENT_BINARY_DIR}/fdict.inc"
Expand All @@ -67,6 +70,7 @@ install(

# Globally define a place where we will install
# cmake configuration stuff, *Target.cmake *Version.cmake etc.
# TODO: Do not set cache variables that can conflict with variables in other projects
set(CMAKE_INSTALL_CMAKECONFIG_DIR
"${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME}"
CACHE STRING
Expand All @@ -76,11 +80,7 @@ mark_as_advanced(CMAKE_INSTALL_CMAKECONFIG_DIR)


# Create pkg-config file
configure_file(
"${CMAKE_CURRENT_SOURCE_DIR}/fdict.pc.in"
"${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}.pc"
@ONLY
)
configure_file(fdict.pc.in fdict.pc @ONLY)
install(
FILES
"${CMAKE_CURRENT_BINARY_DIR}/${PROJECT_NAME}.pc"
Expand Down
27 changes: 10 additions & 17 deletions src/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
# Make CMake run the scripts for creating the input files

# The output directory of the module files
set(LIB_MOD_DIR "${CMAKE_CURRENT_BINARY_DIR}/modules")
if(NOT EXISTS "${LIB_MOD_DIR}")
make_directory("${LIB_MOD_DIR}")
endif()


# Do the preprocessing
fdict_fyppify(
Expand Down Expand Up @@ -43,22 +37,21 @@ install(
# Finally define the fdict library
add_library(${PROJECT_NAME} ${fdict_sources})

# Attach the headers to the target
set_target_properties(${PROJECT_NAME}
PROPERTIES
POSITION_INDEPENDENT_CODE ON
Fortran_MODULE_DIRECTORY "${LIB_MOD_DIR}"
)

# Install all modules (omitting the directory)
install(DIRECTORY "${LIB_MOD_DIR}/" DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}")
# TODO: Move to single module interface and move the other modules to subfolder
# This is to avoid name-clashing (similar to how C projects structure their include)
Copy link
Owner

Choose a reason for hiding this comment

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

What do you meen here? Currently all modules from fdict is exposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The convention in C/C++ projects is to install only 1 header file per project with a unique name. In cases where multiple header files are needed (header only libraries for example), than all the header files are in a subdirectory with the unique name of the project. Otherwise when you install another project that has similar header file names, they will overwrite each other and become unusable.

Same convention applies here. There is a potential of overlap with another project that uses variable.mod for example. In this case, simply add both ${CMAKE_Fortran_MODULE_DIRECTORY} and ${CMAKE_Fortran_MODULE_DIRECTORY}/fdict to the target_include_directories(), then depending on which target_link_libraries, the appropriate modules are used.

Copy link
Owner

Choose a reason for hiding this comment

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

Ah, I see.

install(FILES
${CMAKE_Fortran_MODULE_DIRECTORY}/dictionary.mod
${CMAKE_Fortran_MODULE_DIRECTORY}/variable.mod
Copy link
Owner

Choose a reason for hiding this comment

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

Add quotation for paths with spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See earlier disccussion

${CMAKE_Fortran_MODULE_DIRECTORY}/fdict_types.mod
DESTINATION ${CMAKE_INSTALL_INCLUDEDIR})

# Ensure that the target contains the headers/modules/include files
# and that it gets exported with the Targets output
target_include_directories(${PROJECT_NAME}
INTERFACE
$<BUILD_INTERFACE:"${LIB_MOD_DIR}">
$<INSTALL_INTERFACE:"${CMAKE_INSTALL_INCLUDEDIR}">
PUBLIC
$<BUILD_INTERFACE:${CMAKE_Fortran_MODULE_DIRECTORY}>
$<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
)

# Install the library targets
Expand Down