Remove code around libcros USE flag |
|||||||
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?
,
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.
,
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.
,
May 8 2017
,
Jun 2 2017
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.
,
Jun 2 2017
I would rather these changes wait, until we merge the android side first.
,
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.
,
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.
,
Jun 2 2017
,
Jul 28 2017
,
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
,
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
,
Aug 11 2017
,
Jan 22 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by senj@chromium.org
, May 2 2017