Skip to content

Conversation

@cmuck
Copy link
Contributor

@cmuck cmuck commented Feb 8, 2017

  • Reusing the project name which avoids redeclaration and typos
  • Using include directories instead of target include directories
    for each target
  • Removed FILE GLOB because it is evil and not recommended by Kitware
  • Reformatted file

* Reusing the project name which avoids redeclaration and typos
* Using include directories instead of target include directories
  for each target
* Removed FILE GLOB because it is evil and not recommended by Kitware
* Reformatted file
@pmai
Copy link
Contributor

pmai commented Feb 9, 2017

Note that this breaks the build for osi-sensor-model-packaging (and all other users of OSI) for a couple of reasons:

  • include_directories does not add the include directories as PUBLIC, hence targets in other files (at higher levels of the directory hierarchy) that add the library via target_link_libraries will not automatically add the include directories.
  • Removing the cached variable for the generated include files breaks the build, again since include_directories are not propagated.

Both problems can be remedied by switching the include_directories back to target_include_directories with PUBLIC, which would also remove the need for the ugly cached variable.

@ghost ghost requested a review from pmai February 9, 2017 09:33
@ghost ghost added this to the Open Simulation Interface, preliminary freeze milestone Feb 9, 2017
@ghost ghost requested a review from mbinna February 9, 2017 09:35
@ghost
Copy link

ghost commented Feb 9, 2017

@pmai I am out of this "Titan-Battle" 🗡️

I guess you are adjust the suggestion of @cmuck to fit all use cases.

Copy link
Contributor

@pmai pmai left a comment

Choose a reason for hiding this comment

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

Could be merged with the attached changes...

CMakeLists.txt Outdated

protobuf_generate_cpp(PROTO_SRCS PROTO_HEADERS ${OSI_PROTO_FILES})

include_directories(${PROTOBUF_INCLUDE_DIRS})
Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. if you switch these two include_directories to

target_include_directories(${PROJECT_NAME} PUBLIC ${PROTOBUF_INCLUDE_DIR})
target_include_directories(${PROJECT_NAME} PUBLIC ${CMAKE_CURRENT_BINARY_DIR})
target_include_directories(${PROJECT_NAME}_pic PUBLIC ${PROTOBUF_INCLUDE_DIR})
target_include_directories(${PROJECT_NAME}_pic PUBLIC ${CMAKE_CURRENT_BINARY_DIR})

or a loop to this effect I think the change could be merged.

@cmuck
Copy link
Contributor Author

cmuck commented Feb 9, 2017

I don't know which build should be broken because I've tested it with mkdir build && cd build && cmake .. && make and everything was fine.

For re-usage and portability reasons outside of this OSS project (here we don't have a higher hierarchy or other targets - only one single CMakeLists.txt), I've changed it to target_include_directories. Would be great if you could explain how you use this project.

Can't follow why you want to build something with a cleared CMake cache :-/

@pmai
Copy link
Contributor

pmai commented Feb 9, 2017

Sorry for the confusion, the build I was refering to was https://github.com/OpenSimulationInterface/osi-sensor-model-packaging and other users that include OSI as a submodule, which was the reason for the original CMakeLists.txt structuring.

@pmai pmai merged commit d21baa7 into OpenSimulationInterface:master Feb 9, 2017
@pmai
Copy link
Contributor

pmai commented Feb 9, 2017

D'Oh, I missed the now missing library dependencies during review, will fix in new branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants