Migration of bsw libs docan, doip, logger_integration#491
Conversation
DominikAFischer
force-pushed
the
cr-941835
branch
4 times, most recently
from
af8e2cb to
c18b1c6
Compare
christophruethingbmw
left a comment
christophruethingbmw
left a comment
There was a problem hiding this comment.
Ignored the commits from #494, one comment regarding the integration of etl.
|
|
||
| # Default: select based on the target platform via etl_impl_default (S32K148, POSIX) | ||
| # Override: --//libs/bsw/loggerIntegration:etl_impl=//path/to:your_impl | ||
| label_flag( |
There was a problem hiding this comment.
I wonder how this scales? Many libraries will use etl, I think it does not make sense that each library specifies it's own label flag? Shouldn't we have a more generic one at a higher level every library can then depend on?
The etl_profile comes in already via the dependency of //libs/3rdparty/etl to the label flag //libs/3rdparty/etl:etl_profile.
In total we have three things to consider, the etl implemenation itself, the etl_profile.h and the integration specific implementations like print.cpp.
In our project it is currently handled in a way the both etl_profile.h and the implementations are handled via a single cc_library (without dependencies, so no cycle) configured as a label flag called etl_profile. Then there is a central etl library which takes the etl sources and has the etl_profile label flag as a dependency. All users of etl can then simply refer to this library, the fact that it requires a label flag is hiddne.
There was a problem hiding this comment.
That's definitely a valid concern and would make things easier as far as I can see.
It also would be a departure from the current CMake implementation and might even need changes to source code if I'm correct. So, I would suggest that we add this to the list of design discussions and handle it separately?
There was a problem hiding this comment.
Fine for me, we can do it like this, but we should keep track of this to refactor :)
Lib targets: - //libs/bsw/docan:docan - //libs/bsw/doip:doip - //libs/bsw/loggerIntegration:logger_integration - //libs/bsw/storage:storage - //libs/bsw/transport:transport - //platforms/posix/etlImpl:etl_impl - //platforms/s32k1xx/etlImpl:etl_impl Config targets: - //libs/bsw/loggerIntegration:etl_impl (label_flag injection) - //libs/bsw/loggerIntegration:etl_impl_default (closed select) - //libs/bsw/storage:storage_rtos_impl (closed select) Change-Id: I2f273cf5e077793fbe3ba0b405987aed704a831f
DominikAFischer
force-pushed
the
cr-941835
branch
from
c18b1c6 to
7dbaeb0
Compare
2 participants
Migration of bsw libs docan, doip, logger_integration
Lib targets:
Config targets: