Re: [Tarantool-patches] [PATCH v3 1/9] build: add Christian Hansen c-dt to the build

[Safin Timur via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hello Vlad!

On 05.08.2021 2:58, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch!
> 
> On 02.08.2021 02:40, Timur Safin via Tarantool-patches wrote:
>> * Integrated chansen/c-dt parser as 3rd party module to the
>>    Tarantool cmake build process.
>> * Points to tsafin/c-dt instead iof original chansen/c-dt to
>>    have easier build integration, because there is additional
>>    commit which integrated cmake support
> 
> As I said in the previous review, it points at
> https://github.com/tarantool/c-dt.git, not at tsafin/c-dt. Why
> do you keep saying the contrary? Look:
> 
> 	url = https://github.com/tarantool/c-dt.git
> 
> It is 'tarantool', not 'tsafin' here.

Yes, that was oversight, due to several restarts of rebase session
(I guess due to mess generated with combination of --autosquash and git 
rerere)
Now finally applied to this commit in branch as commit 
#4b90de3bb21b9f28bc7c1f8e851d4c95f1b0f191.

> 
> Also, like on the previous review, when I call 'make' and do
> 'git status', I see:
> 
> 	Changes not staged for commit:
> 	  (use "git add <file>..." to update what will be committed)
> 	  (use "git restore <file>..." to discard changes in working directory)
> 	  (commit or discard the untracked or modified content in submodules)
> 		modified:   third_party/c-dt (modified content, untracked content)
> 
> You still didn't fix it. Please, do.
> 

That was not a problem for me, due to the fact I _always_ use 
out-of-source build, and all artifacts generated separately under 
${CMAKE_CURRENT_BINARY_DIR}/third_party/c-dt/, and such artifacts
inside of source tree problem observed only when you do (not very 
idiomatic) in-source build. cmake generated Makefile overwrite original 
c-dt Makefile.

[While we are here, could you please remind me why we recommend to use 
in-source build in our documentation?
And not prefer the recommended and used by majority of industry for what 
matter out-of-source build?]

We could rather may not influence how and where artifacts will be 
generated from inside of our own c-dt/CMakeLists.txt, we should rather 
modify the way how it get used, like, not include it via simple 
add_subdirectory(which is exactly defining this 
${CMAKE_CURRENT_BINARY_DIR}/third_party/c-dt, which would be equal to 
${CMAKE_CURRENT_SOURCE_DIR}/third_party/c-dt for in-source build, but 
rather use some ExternalProject_Add() techniques, which allows to 
redefine generated and binary files localtion. Let me check what is the 
easiest way here...

Thanks,
Timur