Fix proto sync for Chrome OS |
||||
Issue descriptionCurrently, 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.
,
Oct 16
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.
,
Oct 16
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?
,
Oct 16
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.
,
Oct 17
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 :-)
,
Oct 17
+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.
,
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
,
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
,
Oct 22
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.
,
Nov 19
|
||||
►
Sign in to add a comment |
||||
Comment 1 by hendrich@chromium.org
, Oct 15