What do you exactly need from boot_control_chromeos.cc This file does much more to do and we should not just fork the code like this. If you only need GetPartitionNumber(), then you can simply implement it in dlc-service. Its just a few lines of code.

GetPartitionNumber is not enough (it doesn't do anything). I need to calculate current_slot_ which requires to execute entire BootControlChromeOS::Init().

Even then BootControlChromeOS::Init() is not doing anything special. It is calling into rootdev and searches through the partitions to find the one we are booted from. That doesn't seem like too much code to me.

In general we should deduplicate from libbrillo as much as possible. We already have a huge problem when trying to uprev from upstream when libbrillo versions don't match.

I have a PoC CL here for you:https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1166145

It requires at least 200 LoC to implement 'It is calling into rootdev and searches through the partitions to find the one we are booted from.'. 

I understand your concern, but just because two programs do something similar doesn't mean we suddently have to create a third dependency by moving one program into a shared utility. 

But, if you want to share code, I think the proper way would be to create utility functions in libbrillo that does parts of things you want like finding the current partition, etc and then keep the boot_control_chromeos.cc as is (API wise) and change its implementation to use the utilities you added in libbrillo. But, I can't really tell how much benefit will be in there though.

That works. Let's move the utility functions to libbrillo. Thanks!

btw, libbrillo is already a dependency and we're not introducing any new dependency at all.

It's not about the libbrillo dependency itself :) its about the shared piece of code that needs maintenance in two places now (libbrillo and UE) instead of just UE. And we have to be careful we don't break the downstreams if we move functions to libbrillo. ;-)

I'll move unit test properly to ease the worry...

(not to start a debate here... but UE is shared between Android and Chrome OS, it sounds reasonable to have less platform dependent stuff or even unify these two platforms)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/de0f1caf2f42592bd422a00ed928022081c36549

commit de0f1caf2f42592bd422a00ed928022081c36549
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Fri Oct 05 22:43:31 2018

libbrillo: move some functions from update_engine to libbrillo

Implementation is not changed. Certain parameters are added to replace
private variables.

BUG= chromium:890036 
TEST=unittest

Change-Id: I5b558bddf119e81b8188464b8ad351eae3e2fe1a
Reviewed-on: https://chromium-review.googlesource.com/1252546
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: Xiaochu Liu <xiaochu@chromium.org>

[modify] https://crrev.com/de0f1caf2f42592bd422a00ed928022081c36549/libbrillo/libbrillo.gypi
[add] https://crrev.com/de0f1caf2f42592bd422a00ed928022081c36549/libbrillo/brillo/update_engine/utils.cc
[add] https://crrev.com/de0f1caf2f42592bd422a00ed928022081c36549/libbrillo/brillo/update_engine/utils_unittest.cc
[add] https://crrev.com/de0f1caf2f42592bd422a00ed928022081c36549/libbrillo/brillo/update_engine/utils.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8c4b2c6b54da967bb5cff8b5f5467559aa1027e2

commit 8c4b2c6b54da967bb5cff8b5f5467559aa1027e2
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Sat Oct 06 06:13:33 2018

libbrillo: define update engine DLC constants in libbrillo

We define these constants here so it could be shared by update_engine
and dlcservice.

BUG= chromium:890036 
TEST=emerge-kefka libbrillo

Change-Id: I2af70b3f205304d101d1de35cc01b9bf1e745cbb
Reviewed-on: https://chromium-review.googlesource.com/1266235
Commit-Ready: Xiaochu Liu <xiaochu@chromium.org>
Tested-by: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>

[modify] https://crrev.com/8c4b2c6b54da967bb5cff8b5f5467559aa1027e2/libbrillo/brillo/update_engine/utils.cc
[modify] https://crrev.com/8c4b2c6b54da967bb5cff8b5f5467559aa1027e2/libbrillo/brillo/update_engine/utils.h

I think a better approach here would've been to create a common library in the update engine which is exported by the update_engine and is used by the dlcservice. That way we don't need to bother libbrillo for this and put junks in there that no-one else should load or care about.

In that case, a new dependency of update engine is introduced in dlcservice which is not necessary. By moving it to libbrillo, we are not adding any extra dependency at all.

imho, the logic we are moving here is not update engine specific but Chrome OS specific, so it is reasonable to move it to libbrillo. 

I'm working on remove these code in update_engine. I suggest let's make all future changes to libbrillo regarding these migrated code. 

I still don't think this is a good approach. The fact of the matter is projects are dependent on each other. Same here, dlcservice is technically dependent to the update engine anyway. Moving these pieces of code all over the code base in the shared space just creates the system more coupled. In addition think about the overhead (even though small) we are adding each time a program (and there are many of them) loads libbrillo to its memory. They are loading things that they absolutely don't care and these are waste in memory.

> imho, the logic we are moving here is not update engine specific but Chrome OS specific, so it is reasonable to move it to libbrillo. 

But, do they really have any other customer other than the update engine and dlcservice at this point? Even if they do a shared library will do it like any other shared library. IMO libbrillo should only contain codes that are widely used between all programs in chrome os. If you look at the libbrillo we can see things like:
daemons/
dbus/
errors/
glib/
http/
imageloader/
message_loops/
minijail/
streams/
strings/
update_engine/

As you can see most of these stuff (except for update_engine and imageloader) are widely used throughout the chrome os code base.

I'm afraid we're gonna eventually move more stuff specially from the common code between AOSP and update engine As we have already done here with functions like SplitPartitionName(). This can be disaster for android when they want to port back stuff into android. Previously at least the libbrillo was outside of the platform2. Now that it is inside, it makes it even harder to do these things.

The cleaner way to do this is to create a static or shared library in the update engine with all these things we want in there and export that library to be used by the dlc service. We can easily add .pc files to be only dependent on these specific libraries in dlcservice. That way the Android doesn't have to care about the changes in the libbrillo. They compile the code as is even without creating a different library.

We should really cherish the relation between the AOSP and Chrome OS in update engine as there has been very impactful changes in both sides that is being used by all parties and we should keep maintaining these changes as painless as possible.

So, I'm ok to temporarily land code to get unblocked, but in the longer term we should move the shared things into a library instead of the libbrillo. We can discuss this offline too if this is not convincing :)

Thanks for sharing your concern. Sounds reasonable (though I hope I could hear them earlier - before my 1st CL is approved and merged). 

Sorry, It was my bad. I didn't think it through and I thought this approach would work fine! I would be happy help creating the library.

No worry at all! (I was just making small complaints that can be ignored. just like this one)

Sounds like in the forseeable future, UE will be a relatively independent package for historical reasons. Any package UE depends on (libchrome, libbrillow, etc.) or package that depends on UE (e.g. dlcservice if we decide do external library) will break eventually or hurt eng velocity due to the extra overhead to resolve all types of conflicts (UE android, UE chromeos, libchrome version, libbrillo version, etc.). 

At this point, I would rather duplicating these code in dlcservice. If in the future there are more services that use these code, a new system service looks like the correct way.

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/update_engine/+/0d6922092d04c4690af07b18d5d24a1893b82eba

commit 0d6922092d04c4690af07b18d5d24a1893b82eba
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Fri Oct 19 19:19:34 2018

update_engine: define dlc constants

Instead of using constants in libbrillo, we move these constants back to
update_engine. The libbrillo constants are going away.

BUG= chromium:890036 
TEST=emerge-kefka update_engine

Change-Id: I6d2fd0e2bd437cf9f90ff28bb7b12e5ae702ab40
Reviewed-on: https://chromium-review.googlesource.com/1289092
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>

[modify] https://crrev.com/0d6922092d04c4690af07b18d5d24a1893b82eba/boot_control_chromeos.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/4e2fd54e39736b4dcdbda339bdd7193a1541f6ba

commit 4e2fd54e39736b4dcdbda339bdd7193a1541f6ba
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Fri Oct 19 19:19:35 2018

libbrillo: remove brillo/update_engine

We no longer need to share these code between update_engine and
dlcservice. Instead we duplicate these code in dlcservice so
update_engine stays independent on its own.

This is a revert of both
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1266235
and
https://chromium-review.googlesource.com/c/chromiumos/platform2/+/1252546.

BUG= chromium:890036 
TEST=FEATURES="test" emerge-kefka libbrillo
CQ-DEPEND=1289092

Change-Id: If812c7718a2ff732102dc17e845d5018555ace4e
Reviewed-on: https://chromium-review.googlesource.com/1289451
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/4e2fd54e39736b4dcdbda339bdd7193a1541f6ba/libbrillo/libbrillo.gypi
[delete] https://crrev.com/b0a96119702b6ebabaa50fac274ba1e5b8272b9c/libbrillo/brillo/update_engine/utils.cc
[delete] https://crrev.com/b0a96119702b6ebabaa50fac274ba1e5b8272b9c/libbrillo/brillo/update_engine/utils_unittest.cc
[delete] https://crrev.com/b0a96119702b6ebabaa50fac274ba1e5b8272b9c/libbrillo/brillo/update_engine/utils.h

We'll allow dlcservice to find current slot on its own - duplicating the code in update engine.