Skip to content

Improve command line parsing, adapt to Eyescale/CMake#533#115

Merged
rdumusc merged 1 commit intoBlueBrain:masterfrom
rdumusc:master
Jan 26, 2017
Merged

Improve command line parsing, adapt to Eyescale/CMake#533#115
rdumusc merged 1 commit intoBlueBrain:masterfrom
rdumusc:master

Conversation

@rdumusc
Copy link
Copy Markdown

@rdumusc rdumusc commented Jan 23, 2017

No description provided.

@rdumusc
Copy link
Copy Markdown
Author

rdumusc commented Jan 24, 2017

retest this please

Comment thread apps/CMakeLists.txt

set(_tide_in ${CMAKE_CURRENT_SOURCE_DIR}/tide)
set(_tide_out ${PROJECT_BINARY_DIR}/bin/tide)
add_custom_command(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not configure_file(@only)? This way it should run only if _tide_in has changed

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good question, I had to investigate more to understand:

  • configure_file(COPYONLY) is executed during CMake configure step, which means that CMake has to be re-run if the file has changed.
  • also, if one does "rm bin/tide", then "ninja tide" doesn't know how to recreate the missing dependency (ninja: error: 'bin/tide', needed by 'apps/CMakeFiles/tide', missing and no known rule to make it).
  • with the custom_command aproach the file is correctly recreated if it is missing, and it is also copied only if it has changed (I checked the exact modification dates).

However, my code was missing "DEPENDS ${_tide_in}" in add_custom_command(), so it did not copy the file again if _tide_in was changed. Now everything works as expected.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ok, good then.

@rdumusc rdumusc merged commit 52f1d03 into BlueBrain:master Jan 26, 2017
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