Android support #1034

riqkum · 2018-12-14T14:37:37Z

build script update required by atkualizr-android
package manager for android
log system integration

src/libaktualizr/logging/boost_logging.cc

codecov-io · 2018-12-14T15:18:13Z

Codecov Report

Merging #1034 into master will decrease coverage by 0.61%.
The diff coverage is 81.81%.

@@            Coverage Diff             @@
##           master    #1034      +/-   ##
==========================================
- Coverage   76.19%   75.58%   -0.62%     
==========================================
  Files         164      158       -6     
  Lines        9860     9355     -505     
==========================================
- Hits         7513     7071     -442     
+ Misses       2347     2284      -63

Impacted Files	Coverage Δ
src/libaktualizr/utilities/utils.h	`90.47% <ø> (ø)`	⬆️
src/libaktualizr/telemetry/telemetryconfig.cc	`100% <ø> (ø)`	⬆️
src/libaktualizr/uptane/managedsecondary.cc	`88% <ø> (ø)`	⬆️
...aktualizr/package_manager/packagemanagerfactory.cc	`84.61% <0%> (-15.39%)`	⬇️
src/libaktualizr/config/config.cc	`96.11% <100%> (+0.42%)`	⬆️
src/libaktualizr/utilities/config_utils.h	`92.75% <100%> (+1.44%)`	⬆️
...ibaktualizr/package_manager/packagemanagerconfig.h	`83.33% <50%> (-16.67%)`	⬇️
src/libaktualizr/utilities/utils.cc	`85.95% <93.33%> (-0.05%)`	⬇️
src/aktualizr_info/main.cc	`0% <0%> (-90.1%)`	⬇️
src/aktualizr_info/aktualizr_info_config.cc	`51.21% <0%> (-39.03%)`	⬇️
... and 37 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 3cd2982...68c5411. Read the comment docs.

patriotyk · 2018-12-17T09:54:07Z

Please update your commit using following command: 'git commit --amend -s', then push with force flag.

src/libaktualizr/config/config.cc

OYTIS · 2018-12-17T10:27:59Z

src/libaktualizr/utilities/config_utils.h

@@ -152,9 +152,19 @@ class BaseConfig {
        continue;
      }
      if (boost::filesystem::is_directory(config)) {
+#if (!defined(ANDROID) || __ANDROID_API__ >= 28)
        for (const auto& config_file : Utils::glob((config / "*.toml").string())) {


Same here, let's just use generic version in both cases.

src/libaktualizr/utilities/utils.cc

OYTIS · 2018-12-17T10:30:04Z

src/libaktualizr/utilities/utils.cc

+std::mutex Utils::storage_root_path_mutex_;
+std::string Utils::storage_root_path_;
+
+void Utils::setStorageRootPath(const std::string &storage_root_path) {


Where/how is it used?

I use it to point aktualizr to an allowed private storage on Android.

src/libaktualizr/logging/boost_logging.cc

CMakeLists.txt

pattivacek · 2018-12-17T15:45:50Z

src/libaktualizr/package_manager/androidmanager.cc

+      return {AndroidInstallationState::STATE_INSTALLED, package_file_path};
+    } else if (ext == ".inprogress") {
+      return {AndroidInstallationState::STATE_IN_PROGRESS, package_file_path};
+    } else if (true /* check ext is actually a hash */) {


Is this still a TODO? What's the challenge there?

No challenge, I'll restore the implementation soon.

pattivacek · 2018-12-17T15:51:00Z

src/libaktualizr/package_manager/androidmanager.h

+  data::InstallOutcome finalizeInstall(const Uptane::Target &target) const override {
+    (void)target;
+    throw std::runtime_error("Unimplemented");
+  }


We might want to just return a positive result instead of throwing if there really is nothing to do here. This function exists for the case when installation requires reboot or some external action before we can really call it complete. Does that apply to the android case?

I did android package manager when finalizeInstall call was not yet introduced. It may leads some changes in the current implementation, I need to check.

lbonn · 2019-01-17T13:02:48Z

src/libaktualizr/utilities/config_utils.h

@@ -144,9 +144,8 @@ class BaseConfig {
        continue;
      }
      if (boost::filesystem::is_directory(config)) {
-        for (const auto& config_file : Utils::glob((config / "*.toml").string())) {
+        for (const auto& config_file : Utils::getDirEntriesByExt(config, ".toml"))


Flagged by clang-tidy: we do not allow braceless for loops.

lbonn · 2019-01-17T13:20:50Z

src/libaktualizr/utilities/utils.h

+#if defined(ANDROID)
+  static void setStorageRootPath(const std::string &storage_root_path);
+  static boost::filesystem::path getStorageRootPath();
+  static std::mutex storage_root_path_mutex_;


Does it really need a mutex? I would have thought that setStorageRootPath() would be called once in early initialization.

I also think we could forego the #if defined here and just make this api available in all cases, with a suitable comment (like "used for Android support").
The code in SafeTempRoot could default to use this path if not empty and attempt to use a temporary directory otherwise.

So, if (!Utils::getStorageRootPath()) instead of #if defined(ANDROID).

Yes, that's a better idea.

lbonn · 2019-01-17T13:33:39Z

src/libaktualizr/package_manager/androidmanager.h

+};
+
+struct AndroidInstallationState {
+  enum State { STATE_NOP, STATE_READY, STATE_IN_PROGRESS, STATE_INSTALLED, STATE_UPDATE };


Another style consideration: we now prefer using enum class and name enum variants according to google's c++ style.

e.g.:

aktualizr/src/libaktualizr/primary/initializer.h

Line 11 in 97214dc

enum class InitRetCode { kOk, kOccupied, kServerFailure, kStorageFailure, kSecondaryFailure, kBadP12, kPkcs11Failure };

lbonn · 2019-01-17T13:52:23Z

src/libaktualizr/package_manager/androidmanager.cc

+}
+
+Uptane::Target AndroidManager::getCurrent() const {
+  std::vector<Uptane::Target> installed_versions;


Looks like there is a lot going on behind the scene between this and the aktualizr-android code. I don't object merging soon since it's platform-specific code but it would be nice to have quick summaries:

what are exactly the roles of the different components and how they interact with the state machine?

do you think the need to structure the code like that came from limitations of our current package manager api (not flexible enough)? If so, do you have suggestions to change it to handle this scenario more nicely?

Existing package manager interface doesn't fit well while aktualizr is in role of a slave within aktualizr-android application. Extra entity (dispatcher) that I've introduced is required in that case as a sync. point between the sides. I did so to minimize interaction complexity and decrease amount of data to be exposed. And it brings me faster to the point I could play with an update mechanism on Android side.

However I have the same doubts about clearness of the current solution. I'll look what I can do to simplify it.

While entire update approach on Android is not well-established yet, I have no precise suggestions about changing package manager api.

Thanks for your summary!

If you don't manage to simplify the integration dramatically, I think it could wait. But some documentation about the complete installation process with details about each actors' interactions would be greatly appreciated (when you think it's ready). Either an external doc or helpful code comments would work.

pattivacek

Thanks for the demo today! Just a couple questions and small issues.

Would you mind rebasing on master once again? Unless @lbonn or @eu-smirnov have other thoughts, we can finally merge this soon.

pattivacek · 2019-03-12T15:38:51Z

src/libaktualizr/utilities/utils.cc

@@ -736,6 +740,12 @@ class SafeTempRoot {
  boost::filesystem::path path;
 };

+std::string Utils::storage_root_path_;
+
+void Utils::setStorageRootPath(const std::string &storage_root_path) { storage_root_path_ = storage_root_path; }


This logic with storage_root_path seems fine, but is setStorageRootPath ever used?

Never mind, just realized it's called by aktualizr-android.

pattivacek · 2019-03-12T15:40:54Z

src/libaktualizr/package_manager/CMakeLists.txt

+    target_sources(package_manager PRIVATE androidmanager.cc)
+endif(ANDROID)
+
+aktualizr_source_file_checks(androidmanager.cc androidmanager.h)


This call to aktualizr_source_file_checks can be combined with the two immediately above it.

pattivacek · 2019-03-12T15:58:10Z

src/libaktualizr/package_manager/androidmanager.cc

+  using qi::_1;
+
+  std::string getprop_output;
+  if (0 == Utils::shell("getprop ota.last_installed_package_file", &getprop_output)) {


Where does this property get set?

See explanation in aktualizr-android.

Great, thanks!

pattivacek · 2019-03-12T16:01:13Z

src/libaktualizr/package_manager/androidmanager.cc

+#include <boost/phoenix/stl/algorithm/transformation.hpp>
+#include <boost/spirit/include/phoenix_container.hpp>
+#include <boost/spirit/include/phoenix_core.hpp>
+#include <boost/spirit/include/qi.hpp>


What was it that required using these boost libraries? I see the code below, but I'm curious while normal C++ string parsing was inadequate.

I'm not aware of all the approaches you're trying to follow in the project, so be specific. Standard library does not offer comparable functionality, and hand-written find/extract would be less expressive than using boost/spirit/Qi.

No problem, I think I've got a good enough handle on it.

lbonn · 2019-03-13T09:49:09Z

I think this is also good to go when we'll have resolved your comments @patrickvacek. The boost spirit added dependency doesn't worry me too much if it stays in the android part of code.

* build script update required by atkualizr-android * package manager for android * log system integration Signed-off-by: Maksim Beznos <maximbns@gmail.com>

It relies on Anroid system in-built property to match installed target. Signed-off-by: Maksim Beznos <maximbns@gmail.com>

Signed-off-by: Maksim Beznos <maximbns@gmail.com>

pattivacek

Gitlab actually succeeded despite what Github says, and the LGTM check is bogus and irrelevant. So I think this is ready to go.

Thanks for your patience with keeping this going and seeing it through, @riqkum!

It was added in #1034 for the Android support, but it doesn't appear to be used anywhere. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

riqkum requested review from patriotyk, lbonn and pattivacek December 14, 2018 14:37

lbonn reviewed Dec 14, 2018

View reviewed changes

src/libaktualizr/logging/boost_logging.cc Outdated Show resolved Hide resolved

OYTIS reviewed Dec 17, 2018

View reviewed changes

pattivacek reviewed Dec 17, 2018

View reviewed changes

riqkum added the not-ready label Jan 9, 2019

riqkum force-pushed the feat/android-support branch from caa76bf to aa4f3b4 Compare January 16, 2019 21:17

riqkum added ready and removed not-ready labels Jan 16, 2019

lbonn reviewed Jan 17, 2019

View reviewed changes

riqkum added not-ready and removed ready labels Jan 18, 2019

riqkum added ready and removed not-ready labels Feb 11, 2019

riqkum force-pushed the feat/android-support branch 2 times, most recently from 01a70d6 to e117009 Compare February 15, 2019 09:34

pattivacek reviewed Mar 12, 2019

View reviewed changes

riqkum added 5 commits March 13, 2019 11:58

Android support

ade84e2

* build script update required by atkualizr-android * package manager for android * log system integration Signed-off-by: Maksim Beznos <maximbns@gmail.com>

Android package manager logic is normalized

41cb383

It relies on Anroid system in-built property to match installed target. Signed-off-by: Maksim Beznos <maximbns@gmail.com>

Consider installation result rework

c4c439a

Signed-off-by: Maksim Beznos <maximbns@gmail.com>

Precise prop value parsing to find installed target hash

5c00b06

Signed-off-by: Maksim Beznos <maximbns@gmail.com>

Get the source files checks together

68c5411

Signed-off-by: Maksim Beznos <maximbns@gmail.com>

riqkum force-pushed the feat/android-support branch from e117009 to 68c5411 Compare March 13, 2019 10:13

pattivacek approved these changes Mar 13, 2019

View reviewed changes

pattivacek merged commit ffbed75 into master Mar 13, 2019

pattivacek deleted the feat/android-support branch March 13, 2019 13:01

lbonn mentioned this pull request Apr 29, 2019

Update changelog with recent work. #1188

Merged

pattivacek added a commit that referenced this pull request Apr 20, 2020

Remove dependency on zlib.

db53b10

It was added in #1034 for the Android support, but it doesn't appear to be used anywhere. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek mentioned this pull request Apr 20, 2020

Remove dependency on zlib. #1649

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android support #1034

Android support #1034

riqkum commented Dec 14, 2018

codecov-io commented Dec 14, 2018 •

edited

Loading

patriotyk commented Dec 17, 2018

OYTIS Dec 17, 2018

OYTIS Dec 17, 2018

riqkum Dec 18, 2018

pattivacek Dec 17, 2018

riqkum Dec 18, 2018

pattivacek Dec 17, 2018

riqkum Dec 18, 2018

lbonn Jan 17, 2019

lbonn Jan 17, 2019

riqkum Jan 18, 2019

lbonn Jan 17, 2019

lbonn Jan 17, 2019

riqkum Jan 18, 2019

lbonn Jan 23, 2019

pattivacek left a comment

pattivacek Mar 12, 2019

pattivacek Mar 12, 2019

pattivacek Mar 12, 2019

pattivacek Mar 12, 2019

riqkum Mar 13, 2019

pattivacek Mar 13, 2019

pattivacek Mar 12, 2019

riqkum Mar 13, 2019

pattivacek Mar 13, 2019

lbonn commented Mar 13, 2019

pattivacek left a comment

Android support #1034

Android support #1034

Conversation

riqkum commented Dec 14, 2018

codecov-io commented Dec 14, 2018 • edited Loading

Codecov Report

patriotyk commented Dec 17, 2018

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

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

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

Choose a reason for hiding this comment

lbonn commented Mar 13, 2019

pattivacek left a comment

Choose a reason for hiding this comment

codecov-io commented Dec 14, 2018 •

edited

Loading