Skip to content

lib: include files using known path #16991

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

Closed
wants to merge 2 commits into from
Closed

Conversation

bagder
Copy link
Member

@bagder bagder commented Apr 8, 2025

by including headers using "../[header]" when done from C files in subdirectories, we do not need to specify the lib source dir as an include path and we reduce the risk of header name collisions with headers in the SDK using the same file names.

Ref: #16949

@bagder bagder changed the title lib: include the file using known path lib: include files using known path Apr 8, 2025
@bagder bagder force-pushed the bagder/include-dotdot branch from e9c287c to 6d6df5f Compare April 8, 2025 06:34
@bagder bagder force-pushed the bagder/include-dotdot branch from e459dbc to 6c53a56 Compare April 8, 2025 07:08
@bagder
Copy link
Member Author

bagder commented Apr 8, 2025

@vszakats @icing what do you think about this? This PR makes it a little clearer where include files come from and it reduces the risk of problems with external files having the same names (see #16949) but it adds quite a few ../ into the source files that may feel a little "cluttery".

Personally I think I lean towards this being a small improvement

@testclutch
Copy link

Analysis of PR #16991 at 6c53a560:

Test 3023 failed, which has NOT been flaky recently, so there could be a real issue in this PR.

Generated by Testclutch

@vszakats
Copy link
Member

vszakats commented Apr 8, 2025

The issue this sprung from seems theoretical or artifical even, but,
seeing this change, and how it makes the build setup simpler,
it seems useful to me. While also fixing the theoretical issue.

Suggestion to sync-up this change with cmake builds:

--- a/lib/CMakeLists.txt
+++ b/lib/CMakeLists.txt
@@ -36,10 +36,7 @@ list(APPEND HHEADERS "${CMAKE_CURRENT_BINARY_DIR}/curl_config.h")
 
 # The rest of the build
 
-include_directories(
-  "${PROJECT_BINARY_DIR}/lib"  # for "curl_config.h"
-  "${PROJECT_SOURCE_DIR}/lib"  # for "curl_setup.h"
-)
+include_directories("${PROJECT_BINARY_DIR}/lib")  # for "curl_config.h"
 if(USE_ARES)
   include_directories(SYSTEM ${CARES_INCLUDE_DIRS})
 endif()

bagder added 2 commits April 8, 2025 12:28
by including headers using "../[header]" when done from C files in
subdirectories, we do not need to specify the lib source dir as an
include path and we reduce the risk of header name collisions with
headers in the SDK using the same file names.

Ref: #16949
@bagder bagder force-pushed the bagder/include-dotdot branch from 6c53a56 to a6899ee Compare April 8, 2025 10:29
@bagder bagder marked this pull request as ready for review April 8, 2025 14:12
@icing
Copy link
Contributor

icing commented Apr 8, 2025

I like it. It's more clear.

@bagder bagder closed this in 625f2c1 Apr 8, 2025
@bagder bagder deleted the bagder/include-dotdot branch April 8, 2025 15:01
nbaws pushed a commit to nbaws/curl that referenced this pull request Apr 26, 2025
by including headers using "../[header]" when done from C files in
subdirectories, we do not need to specify the lib source dir as an
include path and we reduce the risk of header name collisions with
headers in the SDK using the same file names.

Idea-by: Kai Pastor

Ref: curl#16949
Closes curl#16991
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants