cmake: Really add FCGI_INCLUDE_DIR to include_directories for rgw #10139
Conversation
|
@theanalyst can you please test to double-check I haven't missed anything? (For reference, the previous PR with the incorrect fix was #9983) |
|
@tserong Thanks works now, lgtm |
| @@ -254,6 +254,7 @@ add_executable(ceph_test_librgw_file | |||
| ) | |||
| set_target_properties(ceph_test_librgw_file PROPERTIES COMPILE_FLAGS | |||
| ${UNITTEST_CXX_FLAGS}) | |||
| target_include_directories(ceph_test_librgw_file PUBLIC ${FCGI_INCLUDE_DIR}) | |||
There was a problem hiding this comment.
nit, PRIVATE would suffice here. as no need to populate ceph_test_librgw_file's INCLUDE_DIRECTORIES to other targets.
|
modulo the nit, lgtm. |
Commit 3cf6c53 was incorrect. FCGI_INCLUDE_DIR doesn't need to be set in src/rgw/CMakeLists.txt, but it does need to be set for the rgw_a target in src/CMakeLists.txt, as well as for the ceph_test_librgw_file and ceph_test_librgw_file_nfsns targets in src/test/CMakeLists.txt. I can only assume that I must not have done a completely clean rebuild at some point when testing a reworked version of the earlier commit :-/ This is only a problem for distros that keep the FCGI headers in /usr/include/fastcgi/ (e.g.: SUSE). This commit also removes a redundant include of <fcgiapp.h> Signed-off-by: Tim Serong <tserong@suse.com>
tserong
force-pushed
the
wip-cmake-really-fix-fcgi-include-dir
branch
from
a5d48b5
to
234b066
Compare
|
De-nitted :-) |
| @@ -1429,6 +1429,7 @@ if(${WITH_RADOSGW}) | |||
| endif(HAVE_SSL) | |||
|
|
|||
| add_library(rgw_a STATIC ${rgw_a_srcs}) | |||
| target_include_directories(rgw_a PUBLIC ${FCGI_INCLUDE_DIR}) | |||
There was a problem hiding this comment.
@tserong, have you tried to change PUBLIC here to PRIVATE? if it fails the build, we should have PUBLIC for sure.
There was a problem hiding this comment.
I just did a rebuild with that line changed from PUBLIC to PRIVATE, and it failed, so I think that one needs to remain as-is.
There was a problem hiding this comment.
@tserong thanks for testing!
|
lgtm. |
|
@theanalyst very sorry, i forgot to add you as the reviewer in the commit message. =( |
|
@tchaikov no problems :) |
Commit 3cf6c53 was incorrect. FCGI_INCLUDE_DIR doesn't need to be
set in src/rgw/CMakeLists.txt, but it does need to be set for the
rgw_a target in src/CMakeLists.txt, as well as for the
ceph_test_librgw_file and ceph_test_librgw_file_nfsns targets in
src/test/CMakeLists.txt. I can only assume that I must not have
done a completely clean rebuild at some point when testing a
reworked version of the earlier commit :-/
This is only a problem for distros that keep the FCGI headers in
/usr/include/fastcgi/ (e.g.: SUSE).
This commit also removes a redundant include of <fcgiapp.h>
Signed-off-by: Tim Serong tserong@suse.com