New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 717306 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Task



Sign in to add a comment

Remove code around libcros USE flag

Project Member Reported by derat@chromium.org, May 1 2017

Issue description

platform2/common-mk/common.gypi defines a USE_libcros variable:

{
  'variables': {
    ...
    'USE_libcros%': 1,

I don't see a libcros USE flag getting set in any overlays. I also don't see the GYP variable getting used anywhere besides update_engine, which uses it to hide some code that uses Chrome to resolve proxies behind ifdefs.

This looks like it was added for http://b/24277309, but the use case described there is obsolete. Can I delete the variable from common-mk and the related ifdefs from update_engine now?
 

Comment 1 by senj@chromium.org, May 2 2017

USE_LIBCROS is set to 0 in Android, I'd prefer to keep it in update_engine code to make it easier to cherrypick/merge changes between the chromeos version and android version.
But I think you can remove it from common.gypi and just set it to 1 in update_engine.gyp.
deymo?

Comment 2 by de...@chromium.org, May 2 2017

yeah, LIBCROS gates whether we compile in support for talking to the interface chrome provides to update_engine or not. The main goal of that was to not compile in support for using that interface if your board doesn't have chrome for example. There are some boards that don't have "chrome" (like jetstream) but I think some of them ended up implementing the chrome interface, not sure if all of them.

In either case, Android sets it to 0. I'm fine with removing it from the gypi's or .ebuild; but you would need to keep the #ifdefs and the -DUSE_LIBCROS=1 in the ChromeOS build.

Comment 3 by derat@chromium.org, May 2 2017

I'm changing the Chrome interface ( issue 446115 ,  issue 703217 ). Do you know who I need to talk to about updating any alternate implementations? I don't see any references to "ResolveNetworkProxy" in the Chrome OS codebase or in g3 apart from the ones that I'm trying to remove from update_engine.

Comment 4 by derat@chromium.org, May 8 2017

Components: OS>Systems

Comment 5 by derat@chromium.org, Jun 2 2017

Cc: sjg@chromium.org
Labels: Hotlist-GoodFirstBug
Owner: ----
Status: Available (was: Assigned)
Summary: Remove code around libcros USE flag (was: Remove code around libcros USE flag if it's unused)
I think we can do the following:

a) Replace all of the #if USE_LIBCROS guards in aosp/system/update_engine with #if defined(__CHROMEOS__) (which is already unconditionally defined in update_engine.gyp). Can we remove the defined(__BRILLO__) stuff as well?

b) Remove the references to USE_LIBCROS, BRILLO_USE_LIBCROS, and USE_libcros from update_engine.gyp and Android.mk.

c) Remove USE_libcros from common.gypi.

Removing references to "libcros" is good because libcros (the shared library) has been gone for many years and org.chromium.LibCrosService (the D-Bus service) is going away soon in favor of fine-grained services exported by Chrome ( issue 692246 ).

Does that all sound correct?

Note that most of this work should wait until the latest Android-side update_engine code is merged to the Chrome OS branch. It'd probably be safe to make the change described in comment #1 now, though.
I would rather these changes wait, until we merge the android side first.

Comment 7 by senj@chromium.org, Jun 2 2017

I would prefer to keep the USE_* flag instead of using __CHROMEOS__, this way the code will be gated by features not platform, maybe some chromeos based OS (jetstream?) don't want this and they can just disable it.
If the reason for removing this flag is libcros has gone, then I think it's better to give the flag a proper name.
For __BRILLO__, there's only one instance in libcurl_http_fetcher.cc, and I think you can get rid of that, just use USE_OMAHA instead.
You can definitely remove BRILLO_USE_LIBCROS.

Comment 8 by derat@chromium.org, Jun 2 2017

Okay, sounds fine to me. If we want to stick with USE_*, I'd recommend splitting it into two #defines, e.g. USE_CHROME_NETWORK_PROXY and USE_CHROME_KIOSK_APP, to match the two separate D-Bus services that will be provided by Chrome.

Comment 9 by derat@chromium.org, Jun 2 2017

Cc: teravest@chromium.org
Owner: ahass...@chromium.org
Status: Started (was: Available)
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 4 2017

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

commit 47186290f8b02cca27574188d72d49f3841f9f4e
Author: Amin Hassani <ahassani@google.com>
Date: Fri Aug 04 22:39:08 2017

update_engine: Breaks the use of libcros into two dbus proxies.

This CL breaks USE_LIBCROS into two defines:
	USE_CHROME_NETWORK_PROXY
	USE_CHROME_KIOSK_APP
and changes all related build artifacts for both chrome and android.

Breaks USE_libcros into USE_chrome_network_proxy and USE_chrome_kiosk_app.
Removes BRILL_USE_LIBCROS.
Replaces __BRILLO__ with USE_OMAHA.

BUG= chromium:717306 
TEST=Ran test 'cros_workon_make --board=amd64-generic --test update_engine'
	for all four conditions of the newly introduced two flags.

Change-Id: I9ca5b35c22a17c45a861db6a434239096a896127
Reviewed-on: https://chromium-review.googlesource.com/596802
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Tested-by: Sen Jiang <senj@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>
Reviewed-by: Sen Jiang <senj@chromium.org>

[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/real_system_state.h
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/real_system_state.cc
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/update_attempter_unittest.cc
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/update_manager/real_system_provider.h
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/update_manager/real_system_provider_unittest.cc
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/update_manager/real_system_provider.cc
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/Android.mk
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/update_manager/state_factory.cc
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/update_engine.gyp
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/update_attempter.h
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/libcurl_http_fetcher.cc
[modify] https://crrev.com/47186290f8b02cca27574188d72d49f3841f9f4e/update_attempter.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 8 2017

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

commit 738e2435c5e018849d0bf762240c9da51b450df7
Author: Amin Hassani <ahassani@google.com>
Date: Tue Aug 08 02:29:14 2017

common-mk: remove USE_libcros

update_engine was the last project that was using USE_libcros, but once
CL:596802 is merged, it is not used anymore and it should be deleted.

BUG= chromium:717306 
TEST=./build_images --board=amd64-generic
CQ-DEPEND=CL:596802

Change-Id: I06c38bceeae3cd0362eb1da0baa642451beef6e9
Reviewed-on: https://chromium-review.googlesource.com/602879
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/738e2435c5e018849d0bf762240c9da51b450df7/common-mk/common.gypi

Status: Fixed (was: Started)

Comment 14 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Sign in to add a comment