-
Notifications
You must be signed in to change notification settings - Fork 0
Allow multiple cpp files in apps
and libs
directories.
#41
Conversation
…nd generated CMake files.
Will close #35 |
src/cmake_templates.cpp
Outdated
km::mustache cmake::add_library_template = {"add_library({{target_name}} ${CMAKE_CURRENT_SOURCE_DIR}/../../../libs/{{obj_name}}/{{obj_source}})\n\n"}; | ||
|
||
// Note the open_hack value in the following 2 templates. open_hack must be set to "${" as a workaround for Mustache decoding the | ||
// wrong pair in "${{{", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand why do you need this open_hack
. it looks like this is a workaround to use ${
for cmake variables but you also have ${CMAKE_CURRENT_SOURCE_DIR}
without open_hack
. So why is it needed and why ${CMAKE_CURRENT_SOURCE_DIR}
can work without it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mustache searches for the FIRST set of double open braces. So it finds the first pair following the dollar sign in this code:
${{{target_name}-source}
^^
resulting in a silent failure when no {target_name
tag can be found.
For more context: when target_name is foo-test
, the final value we want is ${foo-test-source}
; however, there is no way to escape the braces. There is a way to change the macro delimiters, but it isn't any more maintainable in my opinion. Also note that adding a space also fails (i.e. ${ {{target_name}}-source}
) which results in ${ foo-test-source}
).
Because of the need for this workaround and the runtime sanity checking/silent failure, I believe this code should be replaced with the more familiar std::format (or fmt::format until C++20 is our minimum) that allows straightforward escaping and performs compile time checks. To that end I've written #43 to address this problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've got your point. please add tests (or update existing), I tried to reproduce and experiment with this and I just don't know proper command sequence. Anyway I think naming is misleading. It should be something like
{{begin_cmake_variable}}{{target_name}}-source{{end_cmake_variable}}
where
begin_cmake_variable = ${
and end_cmake_variable = }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dimas1185 I have switched the name from open_hack
to the slightly more descriptive dollar_brace
. I believe this along with the comments in the code should be adequate to understand what's going on. Adding a trailing macro resolution (i.e. end_cmake_variable
) feels to me like it only obscures the real problem further.
I additionally added some samples in #43 to show how this might look using std::format.
Finally, I added a test. :-)
I also wrote #46 to inform users when CDT doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for tests.
what about something like this:
s << temp.render(datum{"obj_name", obj.name()}
("target_name", target_name(obj))
("target_name-source", "${" + target_name(obj) + "-source}")
("source_ext", system::extension(obj.language()))
km::mustache cmake::add_contract_template = {
"file(GLOB {{target_name}}-source ${CMAKE_CURRENT_SOURCE_DIR}/../../../apps/{{obj_name}}/*{{source_ext}})\n"
"add_contract({{obj_name}} {{target_name}} {{target_name-source}})\n\n"};
km::mustache cmake::add_library_template = {
"file(GLOB {{target_name}}-source ${CMAKE_CURRENT_SOURCE_DIR}/../../../libs/{{obj_name}}/*{{source_ext}})\n\n"
"add_library({{target_name}} {{target_name-source}})\n\n"};
looks like clearer solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that what's in the code now - while not optiomal - is "good enough". I agree that it's not as good as it should be, and I want to encourage you to review #43 and add comments there.
There's not much point in improving this Mustache code when we are going to remove it within a couple months, right?
Edited for spelling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your last code does look cleaner. But I still think this is lipstick on a pig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see my comment about open_hack
tools/tests/populate_and_build.py
Outdated
assert shutil.copytree(SOURCE_PATH, PROJECT_PATH) | ||
|
||
subprocess.run([APROJ_EXE,"populate", PROJECT_PATH], check=True) | ||
subprocess.run([APROJ_EXE,"build", PROJECT_PATH], check=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also suggest to print full output in case of failure, otherwise that is long debug journey for developer
subprocess.run([APROJ_EXE,"build", PROJECT_PATH], check=True) | |
result = subprocess.run([APROJ_EXE,"build", PROJECT_PATH], capture_output=True, text=True) | |
print(result.stdout) | |
print(result.stderr) | |
sys.exit(result.returncode) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it, I'll add that in tomorrow!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 1a0a18b.
@jolly-fellow @dimas1185 @larryk85 It was less painful to remove it than find a good work around for Mustache. Will close #35 - GLOB source files in cmake. |
Add 'file(GLOB -source /*)' to the 'build' subcommand generated CMake files.
Test with https://github.com/ScottBailey/antler-proj-foo
Add a few line breaks in the generated CMake files for readability/aesthetics.