fix: removed duplicate file #920
Conversation
james-ctc
requested review from
corneliusclaussen,
SebaLukas,
hikinggrass and
Pietfried
as code owners
|
The bazel build fails because EnumFlags.hpp is missing. So you should change the bazel stuff, too. |
james-ctc
force-pushed
the
fix/duplicate-code-removed
branch
from
a3dc05e
to
0380be0
Compare
I'm not sure how bazel works, it doesn't seem to like having relative paths added.
|
|
@james-ctc we can probably help fix bazel but removing it to "fix" cannot a serious idea |
modules/EvseManager/CMakeLists.txt
Outdated
| @@ -21,6 +21,10 @@ target_sources(${MODULE_NAME} | |||
| PersistentStore.cpp | |||
| ) | |||
|
|
|||
| target_include_directories(${MODULE_NAME} PRIVATE | |||
| ../../lib/staging/util | |||
There was a problem hiding this comment.
why don't we just link against the lib/stating/util?
There was a problem hiding this comment.
why don't we just link against the lib/stating/util?
because there are no object files to link against. EnumFlags.hpp is a header file only.
There was a problem hiding this comment.
Did we already consider to create a library target for the lib/staging/util stuff (maybe that's what @dorezyuk means)? The library target could be named something like everest::staging::util. Code like here would consume it then via target_link_libraries(${MODULE_NAME} PRIVATE everest::staging::util).
This works also for header-only libraries.
Furthermore we would decouple the information from where the library is located from the consuming code.
@james-ctc: if you need help with this (cmake related), I could help here
There was a problem hiding this comment.
@a-w50 thank you for the useful information. I've not heard the term header-only library. I'm still relatively new to cmake (used make for 30+ years) so your help is much appreciated.
There was a problem hiding this comment.
@james-ctc I added a commit as a draft on how to include additional libraries (in this case everest::staging::util) including the test via cmake.
It is working now, but I'm not really happy with the namespacing/naming so far. The namespace is quite long. Maybe we can reduce this a bit by using ev instead of everest and staging could become draft. And once it becomes stable it might go to ev::std::util or something similar. Accordingly the include directory structure would need to be adopted.
For now the commit could be taken as is, but soon we should come up with some naming conventions.
There was a problem hiding this comment.
thank you for your help.
please feel free to add a fix - I don't know bazel at all. |
dorezyuk
force-pushed
the
fix/duplicate-code-removed
branch
from
8996aea
to
bd57237
Compare
| @@ -7,7 +7,7 @@ | |||
| #include <atomic> | |||
| #include <type_traits> | |||
|
|
|||
| namespace util { | |||
| namespace everest::staging::util { | |||
|
|
|||
There was a problem hiding this comment.
not sure how you structure it but I would find the include path everest/staging/util more logical
There was a problem hiding this comment.
You're right, for now this would be more consistent. I'll change that
There was a problem hiding this comment.
@dorezyuk I removed the one line from the EvseManager/bazel file, as this should not be needed there
There was a problem hiding this comment.
I see the build works but where do we include it then?
Signed-off-by: James Chapman <[email protected]>
Signed-off-by: James Chapman <[email protected]>
Signed-off-by: aw <[email protected]>
Signed-off-by: aw <[email protected]>
Signed-off-by: aw <[email protected]>
Signed-off-by: Dima Dorezyuk <[email protected]>
Signed-off-by: aw <[email protected]>
Signed-off-by: James Chapman <[email protected]>
james-ctc
force-pushed
the
fix/duplicate-code-removed
branch
from
4808f0b
to
6a1070b
Compare
Describe your changes
EnumFlags.hpp was in two places. Removed duplicate file
Issue ticket number and link
Checklist before requesting a review