cmake: FetchContent/add_subdirectory compatibility improvements by hn-sl · Pull Request #170 · intel/isa-l_crypto

hn-sl · 2026-01-05T05:07:00Z

Summary

This PR replaces CMAKE_SOURCE_DIR with CMAKE_CURRENT_SOURCE_DIR throughout CMake configuration files to enable proper path resolution when the project is consumed via FetchContent or add_subdirectory().

Problem

When isa-l_crypto is used as a subdirectory (via FetchContent or add_subdirectory), CMAKE_SOURCE_DIR points to the parent project's root, not isa-l_crypto's source directory. This causes incorrect path resolution for:

Include directories
configure_file() inputs
NASM assembly include paths

Example Error (FetchContent)

CMake Error: File /path/to/parent_project/cmake/isa-l_crypto.h.in does not exist.
CMake Error: File /path/to/parent_project/cmake/ISALCryptoConfig.cmake.in does not exist.
CMake Error: File /path/to/parent_project/libisal_crypto.pc.in does not exist.

Solution

Replace all occurrences of CMAKE_SOURCE_DIR with CMAKE_CURRENT_SOURCE_DIR, which always refers to the directory containing the current CMakeLists.txt.

Files Changed

CMakeLists.txt
cmake/*.cmake (all module files)

Testing

Verified that the project builds successfully when consumed via:

FetchContent_MakeAvailable()
add_subdirectory()
Standalone build (no regression)

pablodelara · 2026-01-05T08:05:37Z

@hn-sl Thanks for this PR. Could you sign off the commit? Thanks!

Replace CMAKE_SOURCE_DIR with CMAKE_CURRENT_SOURCE_DIR throughout CMake configuration files to enable proper path resolution when the project is consumed via FetchContent or add_subdirectory(). When used as a subdirectory, CMAKE_SOURCE_DIR points to the parent project's root, causing incorrect path resolution for include directories, configure_file inputs, and other paths. Signed-off-by: hsshin <hsshinmail@gmail.com>

hn-sl · 2026-01-05T11:22:34Z

Updated PR Scope

This PR now includes two changes for FetchContent/add_subdirectory compatibility:

1. CMAKE_CURRENT_SOURCE_DIR fix (original)

Replaces CMAKE_SOURCE_DIR with CMAKE_CURRENT_SOURCE_DIR for correct path resolution.

2. Header restructuring (new commit)

Moves headers to include/isa-l_crypto/ subdirectory for consistent include paths:

// Now works for both FetchContent and installed usage
#include <isa-l_crypto/md5_mb.h>

Verified:

Resolves: #171

This enables users to include headers with the standard pattern: #include <isa-l_crypto/md5_mb.h> Changes: - Move all public headers from include/ to include/isa-l_crypto/ - Update CMakeLists.txt and cmake modules for new header paths - Add include/isa-l_crypto to private include paths for internal builds Resolves: intel#171 Signed-off-by: hsshin <hsshinmail@gmail.com>

pablodelara · 2026-01-05T15:12:41Z

Thanks for these updates. I am thinking that include/isa-l_crypto / should only include header files with public API, which are:
aes_cbc.h aes_gcm.h aes_keyexp.h aes_xts.h isal_crypto_api.h md5_mb.h mh_sha1.h mh_sha1_murmur3_x64_128.h mh_sha256.h multi_buffer.h rolling_hashx.h sha1_mb.h sha256_mb.h sha512_mb.h sm3_mb.h

Could you move the rest outside include/isa-l_crypto/ ? Thanks!

hn-sl · 2026-01-05T15:30:11Z

Thanks for the feedback! I analyzed the header dependencies:

Headers included by public API headers (must stay in include/isa-l_crypto/):

types.h - included by md5_mb.h, sha1_mb.h, sha256_mb.h, sha512_mb.h, sm3_mb.h, aes_.h, mh_sha.h, rolling_hashx.h

Headers used only by source files (.c):

Single module usage (can move to that module directory):

md5_mb_internal.h → md5_mb/
sm3_mb_internal.h → sm3_mb/

Multiple module usage (need shared location):

sha1_mb_internal.h → sha1_mb/, fips/
sha256_mb_internal.h → sha256_mb/, fips/
sha512_mb_internal.h → sha512_mb/, fips/
aes_*_internal.h → aes/, fips/
memcpy_inline.h, intrinreg.h → md5_mb/, sha*_mb/, sm3_mb/
test.h, endian_helper.h → almost all modules
aarch64_multibinary.h, riscv64_multibinary.h → almost all modules

Proposed structure:

Single module only → move to that module directory
Multiple modules → move to new internal/ directory
Used by public headers (types.h) → keep in include/isa-l_crypto/

Does this approach work for you?

pablodelara · 2026-01-05T16:22:44Z

Thanks for the feedback! I analyzed the header dependencies:

Headers included by public API headers (must stay in include/isa-l_crypto/):

types.h - included by md5_mb.h, sha1_mb.h, sha256_mb.h, sha512_mb.h, sm3_mb.h, aes__.h, mh_sha_.h, rolling_hashx.h

Headers used only by source files (.c):

Single module usage (can move to that module directory):

md5_mb_internal.h → md5_mb/

sm3_mb_internal.h → sm3_mb/

Multiple module usage (need shared location):

sha1_mb_internal.h → sha1_mb/, fips/

sha256_mb_internal.h → sha256_mb/, fips/

sha512_mb_internal.h → sha512_mb/, fips/

aes_*_internal.h → aes/, fips/

memcpy_inline.h, intrinreg.h → md5_mb/, sha*_mb/, sm3_mb/

test.h, endian_helper.h → almost all modules

aarch64_multibinary.h, riscv64_multibinary.h → almost all modules

Proposed structure:

Single module only → move to that module directory

Multiple modules → move to new internal/ directory

Used by public headers (types.h) → keep in include/isa-l_crypto/

Does this approach work for you?

Makes sense, thanks for looking into it!

- Move multi-module internal headers to internal/ - Move single-module internal headers to their module directories (md5_mb_internal.h -> md5_mb/, sm3_mb_internal.h -> sm3_mb/) - Update CMake, Autotools, and Makefile.unx build systems - Keep only public API headers in include/isa-l_crypto/ - Remove internal headers from umbrella header isa-l_crypto.h Signed-off-by: hsshin <hsshinmail@gmail.com>

pablodelara · 2026-01-06T08:45:07Z

Windows build is failing because internal/ is not in part of INCLUDES in Makefile.nmake. Anyway, I think this internal folder can be moved inside "include/". Also, I think for consistency, we should move sm3_mb_internal.h and sha512_mb_internal.h inside the internal/ folder. And, is it necessary the Makefile.am file inside internal/?

- Move internal/ to include/internal/ - Move md5_mb_internal.h and sm3_mb_internal.h to include/internal/ - Add include/internal/ to Makefile.nmake INCLUDES (fixes Windows build) - Remove internal/Makefile.am (not needed) - Update all build systems (CMake, Autotools, make.inc) Signed-off-by: hsshin <hsshinmail@gmail.com>

hn-sl · 2026-01-06T10:32:20Z

Addressed feedback:

Moved internal/ to include/internal/
Moved md5_mb_internal.h and sm3_mb_internal.h to include/internal/
Added include/internal/ to Makefile.nmake INCLUDES (fixes Windows build)
Removed internal/Makefile.am (not needed - headers are declared via noinst_HEADERS in module Makefile.am files)
Updated all build systems (CMake, Autotools, make.inc)

pablodelara · 2026-01-06T12:55:10Z

Thanks for the update. I'm getting some warnings when compiling on aarch64 on our internal CI.
We should fix this before this can be merged. Thanks.

mh_sha1/aarch64/mh_sha1_aarch64_dispatcher.c: In function '_mh_sha1_update_dispatcher':
./include/aarch64_multibinary.h:204:29: warning: unknown option after '#pragma GCC diagnostic' kind [-Wpragmas]
204 | #define DO_PRAGMA(x) _Pragma(#x)
| ^~~~~~~
./include/aarch64_multibinary.h:205:29: note: in expansion of macro 'DO_PRAGMA'
205 | #define DIGNOSTIC_IGNORE(x) DO_PRAGMA(GCC diagnostic ignored #x)
| ^~~~~~~~~
./include/aarch64_multibinary.h:212:17: note: in expansion of macro 'DIGNOSTIC_IGNORE'
212 | DIGNOSTIC_IGNORE(-Wnested - externs)
| ^~~~~~~~~~~~~~~~
mh_sha1/aarch64/mh_sha1_aarch64_dispatcher.c:56:24: note: in expansion of macro 'PROVIDER_INFO'
56 | return PROVIDER_INFO(mh_sha1_update_ce);

The header move in commit f6d2306 introduced formatting changes that broke GCC pragma warning options: - "-W"#x became "-W" #x (space breaks string concatenation) - -Wnested-externs became -Wnested - externs (invalid warning option) This caused compiler warnings on aarch64 builds. Added clang-format off/on comments to protect these pragma macros from being reformatted by clang-format. Signed-off-by: hsshin <hsshinmail@gmail.com>

hn-sl · 2026-01-06T14:22:12Z

Fixed the aarch64 pragma warning.

The issue was that clang-format reformatted -Wnested-externs to -Wnested - externs (adding spaces around the hyphen), which GCC doesn't recognize as a valid warning option.

Added /* clang-format off */ guards around the pragma macros to prevent clang-format from breaking the pragma string concatenation in the future.

pablodelara

A few comments, thanks for the work!

pablodelara · 2026-01-07T06:32:49Z

include/internal/aarch64_multibinary.h

+                DIGNOSTIC_POP()                                                                    \
+                _func_entry;                                                                       \
+        })
+/* clang-format on */


Better to leave the files as they were and change the path of aarch64_multibinary.h and riscv64_multibinary.h in .clang-format-ignore to the new path.

pablodelara · 2026-01-07T06:50:03Z

aes/Makefile.am

 src_include += -I $(srcdir)/intel-ipsec-mb/lib

-extern_hdrs += include/aes_gcm.h include/aes_cbc.h include/aes_xts.h include/aes_keyexp.h include/isal_crypto_api.h
+extern_hdrs += include/isa-l_crypto/aes_keyexp.h include/isa-l_crypto/isal_crypto_api.h


Any reason to remove these extra header files?

pablodelara · 2026-01-07T06:50:11Z

fips/Makefile.am


 src_include     += -I $(srcdir)/fips
-extern_hdrs     += include/isal_crypto_api.h include/aes_xts.h include/aes_keyexp.h include/sha1_mb.h include/sha256_mb.h
+extern_hdrs += include/isa-l_crypto/sha256_mb.h


Any reason to remove these extra header files?

- Use .clang-format-ignore instead of clang-format off guards for multibinary headers - Restore accidentally dropped headers in other_src sections across all module Makefile.am files (memcpy_inline.h, intrinreg.h, test.h, aes_keyexp_internal.h, aes_cbc_internal.h) - Update header paths to reflect new directory structure (include/isa-l_crypto/ for public, include/internal/ for internal) - Fix isa-l_crypto.h generation to avoid duplicate path prefix (was generating isa-l_crypto/isa-l_crypto/xxx.h instead of isa-l_crypto/xxx.h) Signed-off-by: hsshin <hsshinmail@gmail.com>

hn-sl · 2026-01-07T10:20:12Z

Addressed the review feedback:

Switched to .clang-format-ignore instead of pragma guards
Restored missing headers in other_src sections

Regarding the dropped headers - there was no specific reason. They were accidentally removed during the header path reorganization. Sorry for the oversight.

Also fixed a bug in Makefile.am where the generated isa-l_crypto.h had incorrect include paths. The header generation logic (line 152) was adding an extra isa-l_crypto/ prefix, resulting in paths like <isa-l_crypto/isa-l_crypto/xxx.h> instead of <isa-l_crypto/xxx.h>. This only affected external users who include the umbrella header after installation.

All tests pass (autotools + cmake).

pablodelara · 2026-01-08T11:47:20Z

@hn-sl overall it looks mainly ok except for the isa-l_crypto.h. It currently shows a wrong path when compiling with CMake and Makefile.unx. I'm leaving some comments to fix it.

pablodelara · 2026-01-08T11:49:37Z

CMakeLists.txt

 # Generate isa-l_crypto.h header
 set(ISAL_CRYPTO_HEADER "${CMAKE_BINARY_DIR}/isa-l_crypto.h")
-configure_file(${CMAKE_SOURCE_DIR}/cmake/isa-l_crypto.h.in ${ISAL_CRYPTO_HEADER} @ONLY)
+configure_file(${CMAKE_CURRENT_SOURCE_DIR}/cmake/isa-l_crypto.h.in ${ISAL_CRYPTO_HEADER} @ONLY)


Adding the following lines fix the path problem

--- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -249,6 +249,14 @@ target_include_directories(isal_crypto # Generate isa-l_crypto.h header set(ISAL_CRYPTO_HEADER "${CMAKE_BINARY_DIR}/isa-l_crypto.h") +list(REMOVE_DUPLICATES EXTERN_HEADERS) +list(SORT EXTERN_HEADERS) +set(ISAL_CRYPTO_INCLUDES "") +foreach(HEADER ${EXTERN_HEADERS}) + string(REPLACE "include/" "" HEADER_PATH ${HEADER}) + string(APPEND ISAL_CRYPTO_INCLUDES "#include <${HEADER_PATH}>\n") +endforeach() + configure_file(${CMAKE_CURRENT_SOURCE_DIR}/cmake/isa-l_crypto.h.in ${ISAL_CRYPTO_HEADER} @ONLY)

pablodelara · 2026-01-08T11:53:29Z

cmake/isa-l_crypto.h.in

-#include <isa-l_crypto/test.h>
 #include <isa-l_crypto/types.h>
-#include <isa-l_crypto/endian_helper.h>



All these includes could be replaced by:

+@ISAL_CRYPTO_INCLUDES@

pablodelara · 2026-01-08T11:54:13Z

make.inc

 	@echo '#ifndef _ISAL_CRYPTO_H_'	>> $@
 	@echo '#define _ISAL_CRYPTO_H_'	>> $@
 	@echo ''			>> $@
 	@for unit in $(sort $(extern_hdrs)); do echo "#include <isa-l_crypto/$$unit>" | sed -e 's;include/;;' >> $@; done


This should fix the path:

--- a/make.inc
+++ b/make.inc
@@ -288,7 +288,7 @@ isa-l_crypto.h:
@echo '#ifndef ISAL_CRYPTO_H' >> $@
@echo '#define ISAL_CRYPTO_H' >> $@
@echo '' >> $@

@for unit in $(sort $(extern_hdrs)); do echo "#include <isa-l_crypto/$$unit>" | sed -e 's;include/;;' >> $@; done

@for unit in $(sort $(extern_hdrs)); do echo "#include <$$unit>" | sed -e 's;include/;;' >> $@; done

pablodelara · 2026-01-08T13:27:04Z

@hn-sl I can also make these changes myself, if you are OK with it. I will also condense the commits to 2 or 3.

hn-sl · 2026-01-08T13:32:57Z

Sure, that would be great. Thanks!

pablodelara · 2026-01-08T15:24:50Z

This is now merged, thanks for the work!

hn-sl mentioned this pull request Jan 5, 2026

[CMake] Improve FetchContent compatibility: include path consistency and option naming #171

Closed

hn-sl force-pushed the master branch from 392274a to d942a8e Compare January 5, 2026 08:09

hn-sl changed the title ~~cmake: use CMAKE_CURRENT_SOURCE_DIR for FetchContent compatibility~~ cmake: FetchContent/add_subdirectory compatibility improvements Jan 5, 2026

hn-sl force-pushed the master branch 2 times, most recently from 3e011ce to 85b676c Compare January 5, 2026 12:06

hn-sl force-pushed the master branch from 85b676c to f6d2306 Compare January 5, 2026 13:22

hn-sl force-pushed the master branch from 2a14272 to 62eb0f4 Compare January 6, 2026 14:21

pablodelara reviewed Jan 7, 2026

View reviewed changes

pablodelara reviewed Jan 8, 2026

View reviewed changes

pablodelara closed this Jan 8, 2026

Conversation

hn-sl commented Jan 5, 2026

Summary

Problem

Example Error (FetchContent)

Solution

Files Changed

Testing

Uh oh!

pablodelara commented Jan 5, 2026

Uh oh!

hn-sl commented Jan 5, 2026

Updated PR Scope

1. CMAKE_CURRENT_SOURCE_DIR fix (original)

2. Header restructuring (new commit)

Uh oh!

pablodelara commented Jan 5, 2026

Uh oh!

hn-sl commented Jan 5, 2026

Uh oh!

pablodelara commented Jan 5, 2026

Uh oh!

pablodelara commented Jan 6, 2026

Uh oh!

hn-sl commented Jan 6, 2026

Uh oh!

pablodelara commented Jan 6, 2026

Uh oh!

hn-sl commented Jan 6, 2026

Uh oh!

pablodelara left a comment

Choose a reason for hiding this comment

Uh oh!

pablodelara Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

pablodelara Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

pablodelara Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

hn-sl commented Jan 7, 2026

Uh oh!

pablodelara commented Jan 8, 2026

Uh oh!

pablodelara Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

pablodelara Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

pablodelara Jan 8, 2026

Choose a reason for hiding this comment

Uh oh!

pablodelara commented Jan 8, 2026

Uh oh!

hn-sl commented Jan 8, 2026

Uh oh!

pablodelara commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants