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

Issue 895457 link

Starred by 2 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Task

Blocked on:
issue 842960



Sign in to add a comment

Fix proto sync for Chrome OS

Project Member Reported by pmarko@chromium.org, Oct 15

Issue description

Currently, I can't sync protos from chrome to Chrome OS because device_management_backend.proto contains a reserved enum value.

Reserved enum values are supported since protoc 3.5.0 (which is used in the chrome build process), but not in protoc 3.3.0, which is used in the Chrome OS build process.

We can either try to upgrade protoc in Chrome OS (the 3.3.0 version seems to be due to src/third_party/chromiumos-overlay/dev-libs/protobuf/protobuf-3.3.0.ebuild ), or get rid of the reserved enum value in the chrome device_management_backend.proto and use RESERVED_.* naming to represent that instead.
 
Are you proposing to automatically rewrite all reserved enums to RESERVED_*?
I would suspect upgrading the protoc version is the cleaner solution, I don't know how much effort that may be.

It also fails to build because some enum values have been renamed, but are also used in the code. I haven't looked at the script, but it should have something like "build failed, left branch in current state, please fix manually and continue with ..." in that case.
Nah, not automatically, manually :-) There's currently a single reserved enum value AFAICS. But I agree upgrading protoc would be the nicer solution.

===
Re renamed enum value: Yeah that's ugly but not a huge issue - we can fix the dependent code to depend on the new value.

===
Re script output:
I should make it more explicit, but it is trying to tell me the right thing when I run it:

------
  Sync to Chromium OS
------
Found changes for components/policy.git - upreving
  from 7c04a487ca40373dab9f82a0d098cb3e042c3eac
  to   a7a84eb42b6e427361b4ed50f44555b09537ef7d
Detected changes, upreving...
Creating commit on branch sync_2018_10_15-11_02_02
[sync_2018_10_15-11_02_02 0bf61c753d6] Uprev protofiles and VERSION to Chromium 72.0.3582
 2 files changed, 3 insertions(+), 3 deletions(-)
 rename chromeos-base/protofiles/{protofiles-0.0.24.ebuild => protofiles-0.0.25.ebuild} (98%)
Build Chromium OS and run authpolicy unittests
  log: /usr/local/google/home/pmarko/policy_proto_sync2/runs/run_2018_10_15-11_02_02/chromeos_unit_test.log
build or authpolicy unittests failed.
Build and/or unit tests failed, but it was not the typical
failure.
Please analyze /usr/local/google/home/pmarko/policy_proto_sync2/runs/run_2018_10_15-11_02_02/chromeos_unit_test.log yourself!
For now, we're switching back to master. The uprevved change is
in branch sync_2018_10_15-11_02_02
Note: checking out 'cros/master'.
<more text>

It's lost somewhere in the noise, but it says:
Build and/or unit tests failed, but it was not the typical
failure.
Please analyze /usr/local/google/home/pmarko/policy_proto_sync2/runs/run_2018_10_15-11_02_02/chromeos_unit_test.log yourself!
For now, we're switching back to master. The uprevved change is
in branch sync_2018_10_15-11_02_02

I think if we had color or uppercase text, and said something like "the uprevved change is in branch sync_<blabla> in project src/third_party/chromiumos-overlay, please check it out and manually fix the build", it should be better.
Re reserved enum value:
We would have to do the manual thing every time we run the sync script. And the number of these manual changes will only increase in the future.

Re script output:
Mhhh, I guess I'm just blind :D
Should have been an easy fix then.
Does it automatically re-use the branch when run again or do you have to pass it as a command line parameter somehow?
Re reserved enum value:
I mean, I'd change it in the master version in the chromium repo, and sync that to DMServer and to chromeos. Simply avoid using reserved enum values for now.

Re script output:
It would create a new branch when run again, but basically the bulk of the script's work is done. It has already created a git commit which contains the uprev of the proto dependencies, but the verification step of the git commit failed.
It has left the git commit on the branch.

So the thing to do now is to find out why it doesn't work, fix that, and upload it. The script can't really help here, becuase the issue could be anything. So after fixing the issue, the user can checkout the branch and repo upload . --cbr to upload it as a CL. 
Blockedon: 842960
Upreving protobuf turns out to be a bigger effort (see bug 842960, which is blocked on upgrading portage in  bug 874977 ).

I'll go for the RESERVED_ rename for now as a workaround while experts work on those parts :-)
Cc: hunyadym@chromium.org
+Marton as I think you're waiting for this (?)
I now have 
https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1286551

with a workaround until the protobuf uprev is done.
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 19

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

commit 5bea482112a99c280894ce0a86d83ea3ba2ffa15
Author: Pavol Marko <pmarko@chromium.org>
Date: Fri Oct 19 19:19:31 2018

libbrillo: Follow rollback policy enum value rename

Apply the rename originally performed in http://crrev.com/c/1196376 to
libbrillo unit tests.

BUG=chromium:895457
CQ-DEPEND=CL:1286551
TEST=cros_run_unit_tests --board=amd64-generic --packages libbrillo

Change-Id: I8ac6d737022513d82be0beb7cfa66a12d6689ef7
Reviewed-on: https://chromium-review.googlesource.com/1290689
Commit-Ready: Pavol Marko <pmarko@chromium.org>
Tested-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/5bea482112a99c280894ce0a86d83ea3ba2ffa15/libbrillo/policy/tests/libpolicy_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 19

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

commit 8f4199fdc3462226ca8726658c6259e3f2d51886
Author: Pavol Marko <pmarko@chromium.org>
Date: Fri Oct 19 19:19:29 2018

authpolicy: Adapt for protofiles-0.0.25 version

Adapt to the protofiles-0.0.25 sync.
This requires two changes in authpolicy:
(*) Apply the rename of rollback policy enum values originally performed
    in http://crrev.com/c/1196376 to authpolicy unit tests.
(*) Add support for kDeviceUnaffiliatedCrostiniAllowed in the
    device_policy_encoder.

BUG=chromium:895457
CQ-DEPEND=CL:1286551
TEST=cros_run_unit_tests --board=amd64-generic --packages authpolicy

Change-Id: I977458e594da0ee1d84dc2bc16a4a20f77670ba4
Reviewed-on: https://chromium-review.googlesource.com/1290691
Commit-Ready: Pavol Marko <pmarko@chromium.org>
Tested-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Roman Sorokin <rsorokin@chromium.org>

[modify] https://crrev.com/8f4199fdc3462226ca8726658c6259e3f2d51886/authpolicy/policy/device_policy_encoder.cc
[modify] https://crrev.com/8f4199fdc3462226ca8726658c6259e3f2d51886/authpolicy/policy/device_policy_encoder_unittest.cc

BTW the actual sync landed in https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1286551 - I forgot to mention this bug on that CL. 

Leaving this open so we can remove the patch from the protofiles ebuild when protobuf uprev in bug 842960 has been done.
Status: Assigned (was: Started)

Sign in to add a comment