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

Jump to bottom

Quote path variables that may contain spaces. #84

Merged
merged 1 commit into from
May 24, 2016
Merged

Quote path variables that may contain spaces. #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ set(PROBE_DEVICE_NAME "sysdig")

set(CMD_MAKE make)

set(SYSDIG_DIR ${PROJECT_SOURCE_DIR}/../sysdig)
set(SYSDIG_DIR "${PROJECT_SOURCE_DIR}/../sysdig")

include(ExternalProject)

Expand Down Expand Up @@ -151,7 +151,7 @@ ExternalProject_Add(lpeg
DEPENDS luajit
URL "http://s3.amazonaws.com/download.draios.com/dependencies/lpeg-1.0.0.tar.gz"
URL_MD5 "0aec64ccd13996202ad0c099e2877ece"
BUILD_COMMAND LUA_INCLUDE=${LUAJIT_INCLUDE} ${PROJECT_SOURCE_DIR}/scripts/build-lpeg.sh
BUILD_COMMAND LUA_INCLUDE=${LUAJIT_INCLUDE} "${PROJECT_SOURCE_DIR}/scripts/build-lpeg.sh"
Copy link

Choose a reason for hiding this comment

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

How do you think about to enclose also this include parameter by double quotes?

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 could be wrong, but I though it wasn't necessary as LUAJIT_INCLUDE is defined in terms of LUAJIT_SRC, which is quoted:

set(LUAJIT_INCLUDE "${LUAJIT_SRC}")

Copy link

Choose a reason for hiding this comment

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

I suggest to apply quoting rules consistently. It does not matter that the previous assignment of the mentioned variable is fine while its use for an other parameter might be a bit unsafe.

BUILD_IN_SOURCE 1
CONFIGURE_COMMAND ""
INSTALL_COMMAND "")
Expand Down Expand Up @@ -180,9 +180,9 @@ ExternalProject_Add(lyaml
install(FILES falco.yaml
DESTINATION "${DIR_ETC}")

add_subdirectory(${SYSDIG_DIR}/driver ${PROJECT_BINARY_DIR}/driver)
add_subdirectory(${SYSDIG_DIR}/userspace/libscap ${PROJECT_BINARY_DIR}/userspace/libscap)
add_subdirectory(${SYSDIG_DIR}/userspace/libsinsp ${PROJECT_BINARY_DIR}/userspace/libsinsp)
add_subdirectory("${SYSDIG_DIR}/driver" "${PROJECT_BINARY_DIR}/driver")
add_subdirectory("${SYSDIG_DIR}/userspace/libscap" "${PROJECT_BINARY_DIR}/userspace/libscap")
add_subdirectory("${SYSDIG_DIR}/userspace/libsinsp" "${PROJECT_BINARY_DIR}/userspace/libsinsp")

add_subdirectory(rules)
add_subdirectory(scripts)
Expand Down
8 changes: 4 additions & 4 deletions scripts/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
file(COPY ${PROJECT_SOURCE_DIR}/scripts/debian/falco
DESTINATION ${PROJECT_BINARY_DIR}/scripts/debian)
file(COPY "${PROJECT_SOURCE_DIR}/scripts/debian/falco"
DESTINATION "${PROJECT_BINARY_DIR}/scripts/debian")

file(COPY ${PROJECT_SOURCE_DIR}/scripts/rpm/falco
DESTINATION ${PROJECT_BINARY_DIR}/scripts/rpm)
file(COPY "${PROJECT_SOURCE_DIR}/scripts/rpm/falco"
DESTINATION "${PROJECT_BINARY_DIR}/scripts/rpm")
8 changes: 4 additions & 4 deletions userspace/falco/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
include_directories(${PROJECT_SOURCE_DIR}/../sysdig/userspace/libsinsp/third-party/jsoncpp)
include_directories("${PROJECT_SOURCE_DIR}/../sysdig/userspace/libsinsp/third-party/jsoncpp")
Copy link

Choose a reason for hiding this comment

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

Would it be better to call this CMake command only once with a longer parameter list?

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'm not sure it's a big difference, is it?

Copy link

@elfring elfring May 25, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest to consider another coding style issue.

  • Do you eventually care also for run time characteristcs of CMake commands?
  • Is the build script easier to read if it would contain less command repetitions?

include_directories("${LUAJIT_INCLUDE}")

include_directories(${PROJECT_SOURCE_DIR}/../sysdig/userspace/libscap)
include_directories(${PROJECT_SOURCE_DIR}/../sysdig/userspace/libsinsp)
include_directories("${PROJECT_SOURCE_DIR}/../sysdig/userspace/libscap")
include_directories("${PROJECT_SOURCE_DIR}/../sysdig/userspace/libsinsp")
include_directories("${PROJECT_BINARY_DIR}/userspace/falco")
include_directories("${CURL_INCLUDE_DIR}")
include_directories("${YAMLCPP_INCLUDE_DIR}")
include_directories(${DRAIOS_DEPENDENCIES_DIR}/yaml-${DRAIOS_YAML_VERSION}/target/include)
include_directories("${DRAIOS_DEPENDENCIES_DIR}/yaml-${DRAIOS_YAML_VERSION}/target/include")

add_executable(falco configuration.cpp formats.cpp fields.cpp rules.cpp logger.cpp falco.cpp)

Expand Down