ref: little step forward to separate lib aktualizr headers #1688

kbushgit · 2020-06-10T11:45:52Z

refactoring regarding separating libaktualizr headers "internal/external"
Signed-off-by: Kostiantyn Bushko kbushko@intellias.com

codecov-commenter · 2020-06-15T03:16:10Z

Codecov Report

Merging #1688 into master will decrease coverage by 6.91%.
The diff coverage is 83.41%.

@@            Coverage Diff             @@
##           master    #1688      +/-   ##
==========================================
- Coverage   76.54%   69.62%   -6.92%     
==========================================
  Files         195      188       -7     
  Lines       13353    14674    +1321     
==========================================
- Hits        10221    10217       -4     
- Misses       3132     4457    +1325

Impacted Files	Coverage Δ
include/libaktualizr/aktualizr.h	`22.22% <ø> (ø)`
include/libaktualizr/campaign.h	`57.14% <ø> (ø)`
include/libaktualizr/events.h	`70.58% <ø> (ø)`
include/libaktualizr/results.h	`88.46% <ø> (ø)`
include/libaktualizr/utils.h	`93.33% <ø> (ø)`
src/aktualizr_get/main.cc	`0.00% <ø> (ø)`
src/aktualizr_info/aktualizr_info_config.cc	`51.56% <0.00%> (-28.93%)`	⬇️
src/aktualizr_info/aktualizr_info_config.h	`50.00% <ø> (-50.00%)`	⬇️
src/aktualizr_lite/main.cc	`70.19% <ø> (ø)`
src/aktualizr_primary/main.cc	`59.62% <ø> (-20.38%)`	⬇️
... and 115 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ff75c3...f941134. Read the comment docs.

pattivacek · 2020-06-16T07:20:57Z

Looks good so far, thanks! CI is complaining about some header ordering issues. I hope we can still keep C/C++, Boost/external, and internal headers grouped separately as the styleguide recommends. It might just be an issue of spacing. I also saw that make docs failed because of something obscure with the BootloaderConfig. Not quite sure what that's about.

pattivacek · 2020-06-22T14:57:41Z

It looks like the only problem here is with the formatting because of header include ordering issues. This is a bit unfortunate. I would need to look into this a bit to decide what the right way to do this is. @kbushgit or @eu-smirnov do either of you have strong opinions on how we should order these headers and if the stipulations of the Google style guide (and clang-tidy) are appropriate?

pattivacek

Wow, that's a lot of boilerplate. But looks pretty good so far!

Why prefer including the public API headers with angle brackets instead of quotes? I know that might be preferable for other users of the library, but internally, shouldn't we be searching locally for the headers before searching system paths?

pattivacek · 2020-06-23T15:03:06Z

include/libaktualizr/aktualizr.h

+
+namespace api {
+  class CommandQueue;
+}


I forget, does class api::CommandQueue work? I thought it did and it's a little cleaner.

yes we use it, the disadvantage he is that we have to use smart ptr, the reason is that we cant simple uses forward declaration with member objects defined by the value

aktualizr/include/libaktualizr/aktualizr.h

Line 250 in 39f58c5

std::shared_ptr<api::CommandQueue> api_queue_;

Yeah, that's fine, I just meant if we could put this forward declaration on one line by using the scope resolution operator (::) instead of this three-line variant.

Why not a unique_ptr, though?

Why not a unique_ptr, though?

It will cause a compilation error:

Invalid application of ‘sizeof’ to incomplete type ‘api::CommandQueue’ static_assert(sizeof(_Tp)>0,

It can be fixed by adding an empty dtor for Aktualizr.

Here is an answer with a good explanation
https://stackoverflow.com/a/9954553

pattivacek · 2020-06-23T15:05:34Z

include/libaktualizr/config.h

+#include <libaktualizr/logging_config.h>
+#include <libaktualizr/packagemanagerconfig.h>
+#include <libaktualizr/storage_config.h>
+#include <libaktualizr/telemetryconfig.h>


If we end up having to move all these config files into a common directory, then it no longer makes sense for them to be separate files. We originally separated them out into their respective modules to keep them tied to their relevant code. But if that doesn't work, it's just unnecessary complexity to have them scattered across multiple tiny files.

pattivacek · 2020-06-23T15:28:04Z

src/libaktualizr/config/CMakeLists.txt

@@ -1,4 +1,4 @@
-set(HEADERS config.h)
+#set(HEADERS config.h)


This (and the other commented-out line below) should be removed if they really aren't needed anymore.

pattivacek · 2020-06-23T15:29:14Z

src/libaktualizr/primary/aktualizr.cc

@@ -28,7 +30,8 @@ Aktualizr::Aktualizr(Config config, std::shared_ptr<INvStorage> storage_in,

 void Aktualizr::Initialize() {
  uptane_client_->initialize();
-  api_queue_.run();
+  api_queue_ = std::make_shared<api::CommandQueue>();


Can we move the object creation to the constructor?

kbushgit · 2020-06-24T10:01:22Z

Wow, that's a lot of boilerplate. But looks pretty good so far!

Why prefer including the public API headers with angle brackets instead of quotes? I know that might be preferable for other users of the library, but internally, shouldn't we be searching locally for the headers before searching system paths?

This is a good point and it makes sense but only for the default paths, which are system paths plus current project 's directory. The compiler organizes include paths in the queue and then looking for include files from left to right in that queue, so if we explicitly specify the path using -I (include_directories) option this path will be appended to the left in that queue, so the compiler will be treated user-added path first before look into the system path. In general, I agree that in our nonpublic headers it may be preferred to use quotes instead of angle brackets but not in the public headers. Since I do not see any disadvantages I would prefer to leave that as is and keep using the same approach for both public and private headers.

pattivacek · 2020-06-24T10:18:13Z

Did you mean to close this? :) I assume not.

The compiler organizes include paths in the queue and then looking for include files from left to right in that queue, so if we explicitly specify the path using -I (include_directories) option this path will be appended to the left in that queue, so the compiler will be treated user-added path first before look into the system path. In general, I agree that in our nonpublic headers it may be preferred to use quotes instead of angle brackets but not in the public headers. Since I do not see any disadvantages I would prefer to leave that as is and keep using the same approach for both public and private headers.

I don't think this is completely accurate. At least gcc is explicit that brackets should be used for system headers and quotes for "header files of your own program". My interpretation is that users of libaktualizr will want to include with brackets, but within the project, when referring to our own headers, we should use quotes.

eu-siemann · 2020-06-24T12:06:56Z

While there is indeed no difference between "" and <> if headers are located in a separate include directory, I agree with Patrick that we should use "" for our headers within the project, as it makes it easier to distinguish them.

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

pattivacek · 2020-07-02T08:29:10Z

This was the github CI failure:

2020-07-01T16:30:54.1960145Z In file included from ../src/libaktualizr/bootloader/bootloader_config.cc:2:
2020-07-01T16:30:54.1960458Z In file included from ../src/libaktualizr/utilities/config_utils.h:9:
2020-07-01T16:30:54.1961475Z ../src/libaktualizr/logging/logging.h:8:1: error: class 'LoggerConfig' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Werror,-Wmismatched-tags]
2020-07-01T16:30:54.1962516Z class LoggerConfig;
2020-07-01T16:30:54.1965238Z ^
2020-07-01T16:30:54.1967164Z ../include/libaktualizr/config.h:15:8: note: previous use is here
2020-07-01T16:30:54.1968343Z struct LoggerConfig {
2020-07-01T16:30:54.1969883Z        ^
2020-07-01T16:30:54.1971549Z ../src/libaktualizr/logging/logging.h:8:1: note: did you mean struct here?
2020-07-01T16:30:54.1973115Z class LoggerConfig;
2020-07-01T16:30:54.1974709Z ^~~~~
2020-07-01T16:30:54.1976155Z struct
2020-07-01T16:30:54.1977754Z 1 error generated.

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

pattivacek · 2020-07-02T12:57:39Z

I don't know if you're still unable to see the failures, but here's what I'm seeing:

 In file included from ../src/libaktualizr/primary/aktualizr.cc:5:
../include/libaktualizr/aktualizr.h:15:1: error: class 'SecondaryInfo' was previously declared as a struct; this is valid, but may result in linker errors under the Microsoft C++ ABI [-Werror,-Wmismatched-tags]
class SecondaryInfo;
^
../include/libaktualizr/types.h:443:8: note: previous use is here
struct SecondaryInfo {
       ^
../include/libaktualizr/aktualizr.h:15:1: note: did you mean struct here?
class SecondaryInfo;
^~~~~
struct

and

 /builds/aktualizr-debian-3833209/src/libaktualizr/primary/aktualizr.cc:20:12: error: use '= default' to define a trivial destructor [modernize-use-equals-default,-warnings-as-errors]
Aktualizr::~Aktualizr() {}
           ^            ~~
                        = default;

And it needs a rebase now. If you have clang-tidy-8 on your local machine, I suspect there are going to be more complaints from that, so I encourage running it locally.

pattivacek · 2020-07-02T14:51:30Z

include/libaktualizr/types.h

+ * The intent is to avoid unintentional use of the "naked" relative path by
+ * mandating a base directory for each instantiation.
+ *
+ * @todo has to be moved into Utils namespace


Is this todo still accurate? If so, I'd prefer it were TODO to match all the others in the code. If not, please remove the line.

this syntax is a doxygen compliant, I know we use TODO but what the reason to keep this approach in our public headers!

But is that "todo" more meaningful for the API users reading the documentation or for us, the developers that maintain it and whose burden it would be to actually put it in the Utils namespace?

More importantly, is it actually important for BasedPath to live in the Utils namespace? I'm not convinced it "has to be moved".

pattivacek · 2020-07-02T15:01:52Z

src/libaktualizr-posix/asn1/asn1_message.cc

@@ -12,6 +12,8 @@
 #include <sys/socket.h>
 #include <sys/types.h>

+#include "utilities/utils.h"


The ordering here is mixed up. Seems like it's split into two sections.

pattivacek · 2020-07-02T15:03:11Z

src/libaktualizr/config/CMakeLists.txt

@@ -13,4 +12,5 @@ add_aktualizr_test(NAME config SOURCES config_test.cc ARGS ${PROJECT_BINARY_DIR}
 # config file test for collisions between import and FS->SQL migration paths
 add_test(NAME config-import COMMAND ${PROJECT_SOURCE_DIR}/tests/run_import_clash_test.sh ${PROJECT_SOURCE_DIR}/config)

-aktualizr_source_file_checks(${SOURCES} ${HEADERS} config_test.cc)
+#aktualizr_source_file_checks(${SOURCES} ${HEADERS} config_test.cc)


This commented-out line can be removed.

pattivacek · 2020-07-02T15:04:44Z

src/libaktualizr/package_manager/androidmanager.cc

-
-#include "utilities/utils.h"
-
+:


pattivacek · 2020-07-02T15:06:47Z

src/libaktualizr/utilities/CMakeLists.txt

            dequeue_buffer.h
            exceptions.h
            fault_injection.h
            sig_handler.h
            timer.h
-            types.h
-            utils.h
+	    utils.h


Indenting is off in these two changed lines.

pattivacek · 2020-07-02T15:08:29Z

src/virtual_secondary/partialverificationsecondary.h

@@ -4,12 +4,16 @@
 #include <string>
 #include <vector>

+#include <string>
+#include <vector>


Duplicated.

kbushgit · 2020-07-08T06:55:13Z

/builds/aktualizr-debian-3833209/src/libaktualizr/primary/aktualizr.cc:20:12: error: use '= default' to define a trivial destructor [modernize-use-equals-default,-warnings-as-errors]
Aktualizr::~Aktualizr() {}
^ ~~
= default;
And it needs a rebase now. If you have clang-tidy-8 on your local machine, I suspect there are going to be more complaints from that, so I encourage running it locally.

on my machine installed clang-tidy-8 I don't see such error...

We cant use default destructor in the combination of forwarding declaration and unique_ptr because of this compilation error:

src/libaktualizr-c/CMakeFiles/aktualizr-c.dir/build.make:62: recipe for target 'src/libaktualizr-c/CMakeFiles/aktualizr-c.dir/libaktualizr-c.cc.o' failed
make[2]: *** [src/libaktualizr-c/CMakeFiles/aktualizr-c.dir/libaktualizr-c.cc.o] Error 1
CMakeFiles/Makefile2:15977: recipe for target 'src/libaktualizr-c/CMakeFiles/aktualizr-c.dir/all' failed
make[1]: *** [src/libaktualizr-c/CMakeFiles/aktualizr-c.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
In file included from /usr/include/c++/7/bits/locale_conv.h:41:0,
                 from /usr/include/c++/7/locale:43,
                 from /usr/include/boost/filesystem/path_traits.hpp:29,
                 from /usr/include/boost/filesystem/path.hpp:25,
                 from /usr/include/boost/filesystem.hpp:16,
                 from /home/bush/work/here/origin/master/aktualizr/src/aktualizr_primary/main.cc:3:
/usr/include/c++/7/bits/unique_ptr.h: In instantiation of ‘void std::default_delete<_Tp>::operator()(_Tp*) const [with _Tp = api::CommandQueue]’:
/usr/include/c++/7/bits/unique_ptr.h:263:17:   required from ‘std::unique_ptr<_Tp, _Dp>::~unique_ptr() [with _Tp = api::CommandQueue; _Dp = std::default_delete<api::CommandQueue>]’
/home/bush/work/here/origin/master/aktualizr/include/libaktualizr/aktualizr.h:25:7:   required from here
/usr/include/c++/7/bits/unique_ptr.h:76:22: error: invalid application of ‘sizeof’ to incomplete type ‘api::CommandQueue’
  static_assert(sizeof(_Tp)>0,

kbushgit requested review from pattivacek and eu-siemann June 10, 2020 11:45

kbushgit force-pushed the ref/OTA-3986/libaktualizr-headers branch 2 times, most recently from 12486d5 to cb57f4e Compare June 15, 2020 02:47

kbushgit force-pushed the ref/OTA-3986/libaktualizr-headers branch from cb57f4e to 02a62c7 Compare June 22, 2020 12:16

kbushgit force-pushed the ref/OTA-3986/libaktualizr-headers branch from 02a62c7 to bade364 Compare June 22, 2020 21:55

pattivacek reviewed Jun 23, 2020

View reviewed changes

kbushgit closed this Jun 24, 2020

pattivacek reopened this Jun 24, 2020

kbushgit added 11 commits July 1, 2020 02:21

ref: little step forward to separate lib aktualizr headers

7238405

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

cmake: libaktualizr headers installation

6bdcc52

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

fix include path in tests

f5434e0

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

ref: add/remove headers

8bc23c7

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

ref: finishing definition of aktualizr + move some types into public API

17d9f72

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

Add campaign definition to public api + fix doxygen

79e8b6c

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

adding Target and Hash obj to pub def + del unnecessary includes

f7ea14c

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

put all configs into single header + remove config utils from pub api

0cc5851

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

using unique_ptr with incomplete types requires explicit dtor

62c3051

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

remove definitions of general utils from pub API

cc4742a

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

Remove Config::config_dirs_ member, defined in BaseConfig

b19559e

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

kbushgit force-pushed the ref/OTA-3986/libaktualizr-headers branch from f941134 to b19559e Compare July 1, 2020 16:12

fix: LoggerConfig def + remove unnecessary comments

d73b21a

Signed-off-by: Kostiantyn Bushko <kbushko@intellias.com>

pattivacek reviewed Jul 2, 2020

View reviewed changes

pattivacek mentioned this pull request Jul 7, 2020

ref: moving all configs definition into a single file #1707

Merged

kbushgit closed this Jul 16, 2020

pattivacek deleted the ref/OTA-3986/libaktualizr-headers branch August 7, 2020 09:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ref: little step forward to separate lib aktualizr headers #1688

ref: little step forward to separate lib aktualizr headers #1688

kbushgit commented Jun 10, 2020

codecov-commenter commented Jun 15, 2020 •

edited

Loading

pattivacek commented Jun 16, 2020 •

edited

Loading

pattivacek commented Jun 22, 2020

pattivacek left a comment

pattivacek Jun 23, 2020

kbushgit Jun 24, 2020

pattivacek Jun 24, 2020

eu-siemann Jun 24, 2020

kbushgit Jun 29, 2020

pattivacek Jun 23, 2020

pattivacek Jun 23, 2020

pattivacek Jun 23, 2020

kbushgit commented Jun 24, 2020

pattivacek commented Jun 24, 2020

eu-siemann commented Jun 24, 2020

pattivacek commented Jul 2, 2020

pattivacek commented Jul 2, 2020

pattivacek Jul 2, 2020

kbushgit Jul 7, 2020

pattivacek Jul 8, 2020

pattivacek Jul 2, 2020

pattivacek Jul 2, 2020

pattivacek Jul 2, 2020

pattivacek Jul 2, 2020

pattivacek Jul 2, 2020

kbushgit commented Jul 8, 2020

ref: little step forward to separate lib aktualizr headers #1688

ref: little step forward to separate lib aktualizr headers #1688

Conversation

kbushgit commented Jun 10, 2020

codecov-commenter commented Jun 15, 2020 • edited Loading

Codecov Report

pattivacek commented Jun 16, 2020 • edited Loading

pattivacek commented Jun 22, 2020

pattivacek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbushgit commented Jun 24, 2020

pattivacek commented Jun 24, 2020

eu-siemann commented Jun 24, 2020

pattivacek commented Jul 2, 2020

pattivacek commented Jul 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbushgit commented Jul 8, 2020

codecov-commenter commented Jun 15, 2020 •

edited

Loading

pattivacek commented Jun 16, 2020 •

edited

Loading