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

Issue 825241 link

Starred by 8 users

Issue metadata

Status: Started
Owner:
User never visited
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 921215



Sign in to add a comment

branch-util broken at ToT

Project Member Reported by yueherngl@chromium.org, Mar 23 2018

Issue description

We were trying to branch from recent ToT for factory branch.
The local dry run looks good, with,

cros tryjob branch-util-tryjob --local --branch-name=factory-poppy-10509.B --version=10509.0.0

On the final run,

cros tryjob branch-util --local --production --branch-name=factory-poppy-10509.B --version=10509.0.0

Both of us saw something similar to this,

cbuildbot: Unhandled exception:
Traceback (most recent call last):
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/bin/cbuildbot", line 169, in <module>
    DoMain()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/bin/cbuildbot", line 165, in DoMain
    commandline.ScriptWrapperMain(FindTarget)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/commandline.py", line 911, in ScriptWrapperMain
    ret = target(argv[1:])
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/scripts/cbuildbot.py", line 1019, in main
    logging.info('One stage exited early: %s', ex)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/scripts/cbuildbot.py", line 981, in main
    stack.Add(_SetupConnections, options, build_config)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cros_build_lib.py", line 1331, in Add
    obj = functor(*args, **kwargs)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/scripts/cbuildbot.py", line 803, in _SetupConnections
    db = cidb.CIDBConnectionFactory.GetCIDBConnectionForBuilder()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 2197, in GetCIDBConnectionForBuilder
    return self.GetInstance()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/factory.py", line 104, in GetInstance
    return self._types[self.setup_type]()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/factory.py", line 120, in wrapper
    cached_value.append(function())
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 2090, in <lambda>
    lambda: CIDBConnection(constants.CIDB_PROD_BOT_CREDS)),
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 696, in __init__
    for_service=for_service, query_retry_args=query_retry_args)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 270, in __init__
    listeners=[self._listener_class()])
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/engine/__init__.py", line 387, in create_engine
    return strategy.create(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/engine/strategies.py", line 80, in create
    dbapi = dialect_cls.dbapi(**dbapi_args)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/dialects/mysql/mysqldb.py", line 110, in dbapi
    return __import__('MySQLdb')
ImportError: No module named MySQLdb

We also tried few other versions (e.g. 10504.0.0, 10510.0.0) with the same problem.

Just wondering if we had missed something?

Thanks.
 
Not sure if "apt-get install python-mysqldb" helps, trying it out now.
If this was a standalone tool, MySql wouldn't be needed at all.
RE: #1

Not much better, unfortunately.

Traceback (most recent call last):
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/bin/cbuildbot", line 169, in <module>
    DoMain()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/bin/cbuildbot", line 165, in DoMain
    commandline.ScriptWrapperMain(FindTarget)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/commandline.py", line 911, in ScriptWrapperMain
    ret = target(argv[1:])
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/scripts/cbuildbot.py", line 1019, in main
    logging.info('One stage exited early: %s', ex)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/scripts/cbuildbot.py", line 981, in main
    stack.Add(_SetupConnections, options, build_config)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cros_build_lib.py", line 1331, in Add
    obj = functor(*args, **kwargs)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/scripts/cbuildbot.py", line 803, in _SetupConnections
    db = cidb.CIDBConnectionFactory.GetCIDBConnectionForBuilder()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 2197, in GetCIDBConnectionForBuilder
    return self.GetInstance()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/factory.py", line 104, in GetInstance
    return self._types[self.setup_type]()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/factory.py", line 120, in wrapper
    cached_value.append(function())
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 2090, in <lambda>
    lambda: CIDBConnection(constants.CIDB_PROD_BOT_CREDS)),
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 696, in __init__
    for_service=for_service, query_retry_args=query_retry_args)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 273, in __init__
    temp_engine).fetchall()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 577, in _ExecuteWithEngine
    return self._RunFunctorWithRetries(f)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 596, in _RunFunctorWithRetries
    functor=functor)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/retry_stats.py", line 181, in RetryWithStats
    *args, **kwargs)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/retry_util.py", line 244, in GenericRetry
    return _run()
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/retry_util.py", line 177, in _Wrapper
    ret = func(*args, **kwargs)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/retry_util.py", line 243, in _run
    return functor(*args, **kwargs)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/retry_stats.py", line 168, in wrapper
    result = functor(*args, **kwargs)
  File "/usr/local/google/home/yueherngl/tryjob/repository/chromite/lib/cidb.py", line 573, in <lambda>
    f = lambda: engine.execute(query, *args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/engine/base.py", line 2063, in execute
    connection = self.contextual_connect(close_with_result=True)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/engine/base.py", line 2112, in contextual_connect
    self._wrap_pool_connect(self.pool.connect, None),
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/engine/base.py", line 2151, in _wrap_pool_connect
    e, dialect, self)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/engine/base.py", line 1465, in _handle_dbapi_exception_noconnection
    exc_info
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/util/compat.py", line 203, in raise_from_cause
    reraise(type(exception), exception, tb=exc_tb, cause=cause)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/engine/base.py", line 2147, in _wrap_pool_connect
    return fn()
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/pool.py", line 387, in connect
    return _ConnectionFairy._checkout(self)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/pool.py", line 766, in _checkout
    fairy = _ConnectionRecord.checkout(pool)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/pool.py", line 516, in checkout
    rec = pool._do_get()
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/pool.py", line 1138, in _do_get
    self._dec_overflow()
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/util/langhelpers.py", line 66, in __exit__
    compat.reraise(exc_type, exc_value, exc_tb)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/pool.py", line 1135, in _do_get
    return self._create_connection()
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/pool.py", line 333, in _create_connection
    return _ConnectionRecord(self)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/pool.py", line 461, in __init__
    self.__connect(first_connect_check=True)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/pool.py", line 651, in __connect
    connection = pool._invoke_creator(self)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/engine/strategies.py", line 105, in connect
    return dialect.connect(*cargs, **cparams)
  File "/usr/lib/python2.7/dist-packages/sqlalchemy/engine/default.py", line 393, in connect
    return self.dbapi.connect(*cargs, **cparams)
  File "/usr/lib/python2.7/dist-packages/MySQLdb/__init__.py", line 86, in Connect
    return Connection(*args, **kwargs)
  File "/usr/lib/python2.7/dist-packages/MySQLdb/connections.py", line 204, in __init__
    super(Connection, self).__init__(*args, **kwargs2)
sqlalchemy.exc.OperationalError: (_mysql_exceptions.OperationalError) (2002, 'Can\'t connect to local MySQL server through socket \'/var/run/mysqld/mysqld.sock\' (2 "No such file or directory")')




I can work around the problem by using chromite from older branch (e.g. firmware-glados-7820.B), and do

./cbuildbot branch-util --local --branch-name=factory-poppy-10504.B --version=10504.0.0

to create the branch.

Comment 5 Deleted

I followed comment 4 to create soraka factory branch but I found branches are not created for some repos. For example, src/aosp/system/connectivity/shill and src/aosp/system/update_engine.

Once some CLs need to be cherry-picked, not sure whether we need to manually create the branch.
RE: #6

I thought you were create the branch from R65 branch point? If so and the number of the repositories are small, maybe we can use manually create the branch directly.

Cc: jclinton@chromium.org
Components: Infra>Client>ChromeOS>Build
Labels: -Pri-3 Pri-2
Owner: shapiroc@chromium.org
Summary: branch-util broken at ToT (was: branch creation issue)
Over to Charles to triage for Build area.

See comment #4: need to revive a broken script that was being used to create branches. A few reports on chromeos-infra-discuss, too.

CBuildBot seems like the wrong place for this (note the MySQL connections for no reason). Recently, folks have been running a "cros tryjob --local --production" to invoke the cbuildbot logic on their own machine but reaching out to Prod.

Comment 9 Deleted

Comment 10 by la...@chromium.org, Apr 13 2018

Cc: la...@chromium.org
I think this functionality can probably migrate to the build config service I'm working on.

Comment 11 by lannm@google.com, Apr 16 2018

Owner: la...@chromium.org
One thing to keep in mind...

Current branch util behavior is dependent on a lot of values in the manifest. We used to break it regularly, until branch-util tests were implemented and run on a regular basis (in the master-release builder).

Those failures were hard to diagnose because of the gap in time between landing a manifest change and creating a new branch.

I'm not saying we need the same solution, just that it's an issue to consider.

Comment 13 by lannm@google.com, Apr 16 2018

Thanks, I'll keep an eye on that.

Comment 14 Deleted

Comment 15 Deleted

Comment 16 Deleted

 Issue 833494  has been merged into this issue.
Is this bug still the right place to talk about making a working branch utility for ToT?
Lann, this is related to, but different from build config. This is about actually creating the branches in ChromeOS git repositories.
Labels: Hotlist-GoodFirstBug
Owner: jclinton@chromium.org
Need to give this some attention. Will look at giving to new team member.
Any update on this?
Cc: shapiroc@chromium.org
Charles, I was thinking of giving this to Evan for his starter project. What do you think?
I have a different project.
Owner: vapier@chromium.org
Okay, Mike, as TL can you delegate to a different Build team member?
Don: are you aware of any docs for the branch-util-tryjob ?  the only one i could find is:
  https://sites.google.com/a/google.com/chromeos/resources/engineering/releng/chrom-os-release-engineering-admin-faq

iiuc, the reason we went with a branch-util trybot in the first place (issue 249566) was so that we could isolate ACLs and resources: TPMs didn't need push/branch creation access in their accounts, or a full CrOS checkout, they could simply kick off a remote trybot and it would do everything.  but going by the documentation, TPMs have had to use --local since at least Dec 2013 which pretty much negates the point of doing it in a remote tryjob.

so if there aren't any other advantages to doing it as a tryjob, looks like we should rip the branch_stages logic into a dedicated branch_util script that TPMs can run directly.  if we wanted a remote tryjob, the stages logic could just as easily run the script.

from what i can tell, the "only" thing the script needs to do is:
- have the user specify the manifest version to branch
- have the user specify the branch name
- we load the manifest, process its directives, and create all the refs on the remote server

is there anything else i'm missing here ?
I think you have it.

We can't currently give builders enough permission to create branches without them ALWAYS having enough permissions to create branches and rewrite history, which was deemed a security hole.

That wouldn't be a limitation if we took full advantage of vadimsh@'s service account work.

Note: I once tried ripping the logic out of stages into a standalone script, but found them to be overly tied to cbuildbot. That created enough complexity that I couldn't finish it in the limited time I was willing to invest.

Also note: lannm@'s new sync script will probably prove useful for this.

I think comment 25 is mostly right, there are a couple other options we want to support, like deleting or renaming branches which are occasionally useful to fix mistakes. 
Cc: moragues@chromium.org
Cc: vapier@chromium.org
Labels: OS-Chrome
Owner: dgarr...@chromium.org
Cc: kbleicher@chromium.org
Labels: M-71
I'm being asked to write the new tool?
Owner: vapier@chromium.org
going to get lamont to work on this
Owner: athilenius@chromium.org
Cc: adurbin@chromium.org
Labels: -Pri-2 Pri-1
Can we please get some traction here? I think it's important our branching tooling works w/o dragging around workarounds for months at a time. The original regression was reported back in March.
Owner: evanhernandez@chromium.org
Evan.  the long term solution for this will be integration into the build service.

Can we take a short term look and see if we can fix this on tot currently.
Status: Started (was: Assigned)
How are things going here? Any updates to distill so we can determine where things are at?
This is a pretty important piece of our/any release pipeline - getting branches generated deterministically and correctly.  I sure hope we can get some traction on this.
General update: I've been looking into how the current util works. Also spoke with dgarrett@ a bit about it.

As shapiroc@ said, the long term goal is to merge this utility into the Build Service, which is currently about 2-3 days from running E2E.

dgarrett@ suggested writing a stand-alone script to replace the tryjob as a temporary fix, but I'm hesitant to do this. Writing such a script would still take me a sizable chunk of time (maybe 1-2 weeks). Not to mention, the build service lives in google3 so I doubt we could reuse the script in any useful capacity. In my mind it seems more worthwhile to invest those 2 weeks into migrating the tool entirely to its final resting place. I estimate that this will ~3 work weeks to do properly.

Happy to hear your thoughts. I'll try to be posting weekly updates from here on about the status of the project.
We do have a workaround for creating branches (per #4) so as long as that's not broken, I don't think we are in rush to get the new "feature" ready, IMHO.

Update: last week, the build team changed the scope of the build server, and consequently we've decided to replace the branch tryjob with a standalone script.

You may find the rationale for not fixing the existing job here: go/branch-util-revamp

The interface for the new tool is defined in this CL:
https://chromium-review.googlesource.com/c/chromiumos/chromite/+/1378694

Please comment your concerns about the interface there. The present scope of the tool is for it to do *only* what the original tryjob could do. Additional features may be added but probably next quarter and at a lower priority than simply bringing branch util back to ToT.
Project Member

Comment 43 by bugdroid1@chromium.org, Jan 6

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/0bf2c7eca8d4c76228482f4aadda9edc377629c3

commit 0bf2c7eca8d4c76228482f4aadda9edc377629c3
Author: Evan Hernandez <evanhernandez@chromium.org>
Date: Sun Jan 06 04:08:27 2019

cros branch: Add cros branch command (no impl).

This CL provides an avenue for discussing the new branch
command interface.

TEST=cros branch -h
BUG=chromium:825241

Change-Id: Ifa14803bd193201f110b301781033a2ee105e14f
Reviewed-on: https://chromium-review.googlesource.com/1378694
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Evan Hernandez <evanhernandez@chromium.org>
Reviewed-by: David McMahon <djmm@chromium.org>
Reviewed-by: Bernie Thompson <bhthompson@chromium.org>

[add] https://crrev.com/0bf2c7eca8d4c76228482f4aadda9edc377629c3/cli/cros/cros_branch.py

Project Member

Comment 44 by bugdroid1@chromium.org, Jan 6

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

commit ed43a650f0c063e682bab0019ff786498c2347b6
Author: Evan Hernandez <evanhernandez@chromium.org>
Date: Sun Jan 06 04:08:28 2019

chromite: repo_manifest reads project annotations.

TEST=./repo_manifest_unittest
BUG=chromium:825241

Change-Id: I3c7a97941c4845c9c34cfe2255ead666b8525f85
Reviewed-on: https://chromium-review.googlesource.com/1384764
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Evan Hernandez <evanhernandez@chromium.org>
Reviewed-by: Lann Martin <lannm@chromium.org>

[modify] https://crrev.com/ed43a650f0c063e682bab0019ff786498c2347b6/lib/repo_manifest_unittest.py
[modify] https://crrev.com/ed43a650f0c063e682bab0019ff786498c2347b6/lib/repo_manifest.py

Project Member

Comment 45 by bugdroid1@chromium.org, Jan 8

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

commit af1fe10f99b95a23575cc9db6a30c0c1f26b1fa3
Author: Evan Hernandez <evanhernandez@chromium.org>
Date: Tue Jan 08 19:44:02 2019

cros branch: Locally fetch & branch off manifest.

TEST=./run_tests cli/cros/cros_branch_unittest
BUG=chromium:825241

Change-Id: Ic63165dfa9c6ea4ec765e55e4697b331333c4cc4
Reviewed-on: https://chromium-review.googlesource.com/1381062
Commit-Ready: Evan Hernandez <evanhernandez@chromium.org>
Tested-by: Evan Hernandez <evanhernandez@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>

[add] https://crrev.com/af1fe10f99b95a23575cc9db6a30c0c1f26b1fa3/cli/cros/cros_branch_unittest
[modify] https://crrev.com/af1fe10f99b95a23575cc9db6a30c0c1f26b1fa3/cli/cros/cros_branch.py
[add] https://crrev.com/af1fe10f99b95a23575cc9db6a30c0c1f26b1fa3/cli/cros/cros_branch_unittest.py

Project Member

Comment 46 by bugdroid1@chromium.org, Jan 10

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

commit b2fb7ceaeff383de3565039b40a7e9143d7489e6
Author: Evan Hernandez <evanhernandez@chromium.org>
Date: Thu Jan 10 21:58:30 2019

chromite: repo_manifest exposes includes.

Rather than reimplementing `repo manifest`, this allows
repo_manifest.Manifest to serve as a thin wrapper around
manifest XML files. It will be necessary for repairing
manifests in `cros branch`.

TEST=./run_tests lib/repo_manifest_unittest
BUG=chromium:825241

Change-Id: I6169556896ee56848b28562025091d5fe372eed5
Reviewed-on: https://chromium-review.googlesource.com/1387867
Commit-Ready: Evan Hernandez <evanhernandez@chromium.org>
Tested-by: Evan Hernandez <evanhernandez@chromium.org>
Reviewed-by: Lann Martin <lannm@chromium.org>

[modify] https://crrev.com/b2fb7ceaeff383de3565039b40a7e9143d7489e6/lib/repo_manifest_unittest.py
[modify] https://crrev.com/b2fb7ceaeff383de3565039b40a7e9143d7489e6/lib/repo_manifest.py

Project Member

Comment 47 by bugdroid1@chromium.org, Jan 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/3b5d9e029f3921428fa44445faa11b95fb0c893d

commit 3b5d9e029f3921428fa44445faa11b95fb0c893d
Author: Evan Hernandez <evanhernandez@chromium.org>
Date: Thu Jan 10 21:58:31 2019

cros branch: Partially repair branched manifests.

All branchable projects in the branched manifest must point to the
new branch. The original tryjob did this manually (i.e., it was
not automatically handled by CBuildBot). This CL begins to reimplement
that functionality, relying heavily on the new standard lib for
manifest manipulation, chromite/lib/repo_manifest.

Right now, only project revision is repaired. Other repairs will
be implemented in the next CL.

TEST=./run_tests cli/cros/cros_branch_unittest
BUG=chromium:825241

Change-Id: I36ca58c0f3eb2d08028823a40c335bfa34c20447
Reviewed-on: https://chromium-review.googlesource.com/1401643
Commit-Ready: Evan Hernandez <evanhernandez@chromium.org>
Tested-by: Evan Hernandez <evanhernandez@chromium.org>
Reviewed-by: Evan Hernandez <evanhernandez@chromium.org>

[modify] https://crrev.com/3b5d9e029f3921428fa44445faa11b95fb0c893d/cli/cros/cros_branch.py
[modify] https://crrev.com/3b5d9e029f3921428fa44445faa11b95fb0c893d/cli/cros/cros_branch_unittest.py

Project Member

Comment 48 by bugdroid1@chromium.org, Jan 10

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

commit f043718aa77e60aa66315b4f1f6f4b04df50b60a
Author: Evan Hernandez <evanhernandez@chromium.org>
Date: Thu Jan 10 21:58:31 2019

cros_branch_unittest: Split tests.

BranchCommandTest was beginning to look less like a
unit test and more like a functional test. Split it
up in anticipation of future CLs that add complexity.

TEST=./run_tests cli/cros/cros_branch_unittest
BUG=chromium:825241

Change-Id: Ibe52c9d0e45351ff5816114574ae506747358c4c
Reviewed-on: https://chromium-review.googlesource.com/1401644
Commit-Ready: Evan Hernandez <evanhernandez@chromium.org>
Tested-by: Evan Hernandez <evanhernandez@chromium.org>
Reviewed-by: Lann Martin <lannm@chromium.org>

[modify] https://crrev.com/f043718aa77e60aa66315b4f1f6f4b04df50b60a/cli/cros/cros_branch_unittest.py

Project Member

Comment 49 by bugdroid1@chromium.org, Jan 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/54cd0f1d2270f6f2468bf65faa60d5c1044812db

commit 54cd0f1d2270f6f2468bf65faa60d5c1044812db
Author: Evan Hernandez <evanhernandez@chromium.org>
Date: Thu Jan 10 21:58:31 2019

cros branch: Split helpers for increased testing

CanBranchProject in particular is core to cros branch
and should be tested thoroughly.

TEST=./run_tests cli/cros/cros_branch_unittest
BUG=chromium:825241

Change-Id: I73aefbc546a6a1f361aec09c8532d5efe48b9f8b
Reviewed-on: https://chromium-review.googlesource.com/1401645
Commit-Ready: Evan Hernandez <evanhernandez@chromium.org>
Tested-by: Evan Hernandez <evanhernandez@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>
Reviewed-by: Lann Martin <lannm@chromium.org>

[modify] https://crrev.com/54cd0f1d2270f6f2468bf65faa60d5c1044812db/cli/cros/cros_branch.py
[modify] https://crrev.com/54cd0f1d2270f6f2468bf65faa60d5c1044812db/cli/cros/cros_branch_unittest.py

I don't know if this has been covered.
Can we add a requirement for the new branch-util to fail with an error if a repeated branch point is being specified.

Possibly adding a --force type option, if you really intend to duplicate the build number.

I will capture this in a separate bug.

-Bob
Blocking: 921215
Added crbug:921215 with a feature request.
Thanks for your consideration.
The new tool is already configured to fail when the branch exists remotely. To overwrite the existing branch, you must specify --force. On ToT chromite you should be able to run `cros branch -h` to see the full interface.

Update:
- Remote branch creation is about 2 or 3 CLs away. Right now it can successfully create branches on local checkouts. My primary time sink for remote is writing a functional test.
- Branch renaming and deletion should come shortly after or in parallel. They're much less complicated.

Probably will be some delays because I was out sick yesterday and today, and next week I will be traveling for meetings. I'm shooting to be finished by end of next week if everything goes well, but it could stretch one more week.
To clarify the name of the branch does not have to be identical for it to break things, it merely has to share the version number it branched at with another branch. 

For example if someone generates a branch stabilize-1234.B then someone else comes in and creates release-1234.B then when either builds it will be conflicting with the other over the version space (e.g. is 1234.2.0 from the stabilize or the release?).

The easiest way to do this would be to leverage the naming convention, don't allow the same two numbers to exist in the set of branches (this is what we do by hand today and we should check automatically to start IMO). However this relies on humans to ensure the branch names match so a more comprehensive way would be to check the manifest-versions repo for an existing version being used, though this also has a race in it if the other branch had not tried to build one yet.
Thanks for the clarification.

Branch names are generated in the new tool. Correct me if I am wrong, but it seems sufficient to check if a branch exists with the tag -<major version>.B. If a branch off that version exists, the tool can fail unless --force is specified, in which case it will ask for confirmation.

BTW, I thought minibranches increased the third version number, not the second?
Checking the version in the branch name is probably fine to start, if no one overrides the branch naming convention then this would be sufficiently reliable. 

For the version numbers it depends on where one branches from, if one branches from ToT XXXX.0.0 then one gets XXXX.Y.0, if one branches from XXXX.Y.0 one gets XXXX.Y.Z, one cannot branch XXXX.Y.Z (no 4th digit supported). 

The version numbers are illustrated a bit at https://screenshot.googleplex.com/JyEt65paw18
Project Member

Comment 57 by bugdroid1@chromium.org, Jan 19 (4 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/741cee1e3ff5941bdd5492479b277cb6992373db

commit 741cee1e3ff5941bdd5492479b277cb6992373db
Author: Evan Hernandez <evanhernandez@chromium.org>
Date: Sat Jan 19 04:05:13 2019

cros branch: Completely repair manifest.

Repairing a manfiest means setting the revision
attribute of each project tag to point to the new
branch (if a branch was indeed created for that project)
or to point to the pinned revision (to ToT) if the project
was tagged "pinned" (tagged "ToT").

The original branch-util optimized for manifest
readability by preserving defaults. The new tool
prefers code readability and explicitly sets
project revisions, regardless of whether the
project was branched, pinned, or left on ToT.

TEST=./run_tests cli/cros/cros_branch_unittest
BUG=chromium:825241

Change-Id: I62153c687a8bb148ee0e525642d95f3176721e04
Reviewed-on: https://chromium-review.googlesource.com/1403874
Commit-Ready: Evan Hernandez <evanhernandez@chromium.org>
Tested-by: Evan Hernandez <evanhernandez@chromium.org>
Reviewed-by: Jason Clinton <jclinton@chromium.org>
Reviewed-by: Don Garrett <dgarrett@chromium.org>

[modify] https://crrev.com/741cee1e3ff5941bdd5492479b277cb6992373db/cli/cros/cros_branch.py
[modify] https://crrev.com/741cee1e3ff5941bdd5492479b277cb6992373db/cli/cros/cros_branch_unittest.py

Project Member

Comment 58 by bugdroid1@chromium.org, Jan 19 (4 days ago)

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/chromite/+/74c8e8a018884141f76fe6295b57ea3a4eab850d

commit 74c8e8a018884141f76fe6295b57ea3a4eab850d
Author: Evan Hernandez <evanhernandez@chromium.org>
Date: Sat Jan 19 04:05:13 2019

repo_manifest: _ManifestElement supports del.

`cros branch` needs this to delete attributes during
manifest repair after branching.

TEST=./run_tests lib/repo_manifest_unittest
BUG=chromium:825241

Change-Id: I6cde90e606ad3a4066888ffeaa66bccf73880a47
Reviewed-on: https://chromium-review.googlesource.com/1407821
Commit-Ready: Evan Hernandez <evanhernandez@chromium.org>
Tested-by: Evan Hernandez <evanhernandez@chromium.org>
Reviewed-by: Lann Martin <lannm@chromium.org>

[modify] https://crrev.com/74c8e8a018884141f76fe6295b57ea3a4eab850d/lib/repo_manifest_unittest.py
[modify] https://crrev.com/74c8e8a018884141f76fe6295b57ea3a4eab850d/lib/repo_manifest.py

Sign in to add a comment