fluffi icon indicating copy to clipboard operation
fluffi copied to clipboard

Explicitly list dependencies in makefile or use auto dependency generation

Open maverickwoo opened this issue 4 years ago • 3 comments

The present core/makefile depends on shell tricks to list dependencies. This is error-prone.

Example: A change in core/LocalManager/LMDatabaseManager.h may not trigger a rebuild of the LocalManager binary. Specifically,

https://github.com/siemens/fluffi/blob/6fd185743822c9e48973a426cecd0081ac3e3e86/core/makefile#L168

leads to the var definition in

https://github.com/siemens/fluffi/blob/6fd185743822c9e48973a426cecd0081ac3e3e86/core/makefile#L73-L74

So we know the binary depends on LMDatabaseManager.o because LMDatabaseManager.cpp exists. However, the rule in

https://github.com/siemens/fluffi/blob/6fd185743822c9e48973a426cecd0081ac3e3e86/core/makefile#L164-L166

won't cause a rebuild of LMDatabaseManager.o after LMDatabaseManager.h has been changed because LMDatabaseManager.cpp has indeed not been changed.

For this particular example (and some other similar cases we've seen), there can be some small and more-or-less systematic fixes due to the current inclusion structure. However, a correct long-term solution may call for either of the two ideas in the issue title (while accounting for both Linux and Windows toolchains).

P.S. I am filing this as a way to track the issue; we do not need any systematic fix in the short term. This bug was initially observed by @vinamrabhatia.

maverickwoo avatar May 19 '20 21:05 maverickwoo

I am aware the current makefile has several shortcomings, including the header dependency issues you describe. It has, as many things have, grown over the course of the project to be too complex and confusing to maintain properly. We should mid to long term replace it. I intend to move the whole build to cmake in the future, but my time is currently limited. Let's keep this open as a feature to be implemented down the line.

p0wer0xff avatar May 26 '20 14:05 p0wer0xff

Yes, we also think moving to cmake in the long term is a great idea. Internally, we are slowly growing our CMakeLists.txt; that was forced because we use CLion, which defines a project using CMakeList.txt, and we want auto-completion and definition look-up to work inside the IDE.

I think over time this effort may evolve into something that we can base the fix to this PR on. This is especially after seeing the bug above, which has convinced me there is something to fix. Let's wait a bit and see.

maverickwoo avatar May 26 '20 16:05 maverickwoo

I just added the help wanted label. Any help migrating our current make file to cmake is welcomed :)

TomSie avatar May 27 '20 03:05 TomSie