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

[Vladislav Shpilevoy]

On 05.08.2021 11:55, Safin Timur wrote:
> 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?]

No idea. I personally use in-source build because I have just used to it.

> 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...

I am not a cmake master, so I don't know a correct way to fix it. But I
don't understand why do you need Makefile in c-dt at all if it works via
cmake anyway and 100% of that Makefile is simply overwritten. It is not
used. If a file is not used and is overwritten anyway, why not drop it
from the repository?

Also I see

	Untracked files:
	  (use "git add <file>..." to include in what will be committed)
		libcdt.a

You need to update gitignore so as there is no the warning.