Closed
Bug 1365181
Opened 7 years ago
Closed 7 years ago
turn gecko-servo backouts into servo/servo pull requests
Categories
(Developer Services :: Servo VCS Sync, enhancement)
Developer Services
Servo VCS Sync
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: glob, Assigned: glob)
References
Details
Attachments
(16 files, 2 obsolete files)
(deleted),
text/x-review-board-request
|
Details | |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
smacleod
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
gps
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
glob
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
glob
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
smacleod
:
review+
|
Details |
we need to morph backouts from the servo/ directory on integration/autoland into pull requests on the servo/servo github repository. the current plan is to: - listen for backout commits via pulse - for each backout that touches servo/ - create a pull request with highest priority - link the backed out pull-request and bug with the new pr
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
mozreview-review |
Comment on attachment 8868023 [details] servo-vcs-sync: fix PEP8 warnings due to long lines (bug 1365181) https://reviewboard.mozilla.org/r/139600/#review145166
Attachment #8868023 -
Flags: review+
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8868024 [details] servo-vcs-sync: create the wheelhouse in a shared location (bug 1365181) https://reviewboard.mozilla.org/r/139602/#review145174 The wheelhouse is a cache. So I'd prefer if we could share the cache across multiple virtualenvs. If there is anything to do here, it is probably to make the micro-envs actually use the wheelhouse. I think I forgot to port that feature to the new environment :(
Attachment #8868024 -
Flags: review-
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8868025 [details] servo-vcs-sync: add mozautomation dependancy (bug 1365181) https://reviewboard.mozilla.org/r/139604/#review145176 This would get an r+ if it didn't get bit rotted by dropping the prior commit.
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8868026 [details] servo-vcs-sync: add configuration for servo-backout-pr (bug 1365181) https://reviewboard.mozilla.org/r/139606/#review145178
Attachment #8868026 -
Flags: review+
Comment 11•7 years ago
|
||
mozreview-review |
Comment on attachment 8868027 [details] WIP servo-vcs-sync: extract hg_converted handling into own method (bug 1365181) https://reviewboard.mozilla.org/r/139608/#review145182 ::: vcssync/mozvcssync/servo.py:98 (Diff revision 1) > + on_hg_converted(body) > + > - message.ack() > + message.ack() > - return > > + def on_hg_converted(body): It is really weird to see a reference to a function that occurs later in the same scope. I know this works, but still. Would you mind moving the callbacks to global functions? They exist in the main function mostly out of laziness.
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review145210 Mix of high and low level feedback. I have some additional questions based on what you just said in IRC. But I'm guessing those will be answered in follow-up work. Overall, I like the direction and code architecture. There's a ton of details we need to flush out. If I had one request for v2 it would be for more docs explaining the overall architecture. Since it is still in flux, trying to assess the implementation against the desired end state is a bit difficult! ::: vcssync/mozvcssync/github_pr.py:21 (Diff revision 1) > + self.token = os.getenv('GITHUB_TOKEN') > + if self.token is None: > + raise Exception('GITHUB_TOKEN environmental var is not defined') Please don't use environment variables from library functions. The thing closest to the new process invocation should read environment variables then pass them around as regular variables. Or use config files. ::: vcssync/mozvcssync/github_pr.py:28 (Diff revision 1) > + raise Exception('GITHUB_TOKEN environmental var is not defined') > + self._fork_repo = None > + self._upstream_repo = None > + > + # login to github > + self.github = github3.login(token=self.token) Actually, we may just want to pass a github client instance into the function, as reusing clients can result in perf wins, so that's a behavior we want to encourage. ::: vcssync/mozvcssync/github_pr.py:33 (Diff revision 1) > + self.github = github3.login(token=self.token) > + self.user = str(self.github.user()) > + logger.info('logged in to github as %s' % self.user) > + > + def git(self, *command): > + command = command[0] if type(command[0]) is list else list(command) Nit: can this be `isinstance(command[0], list)`? I almost never see `type(x) is ...` ::: vcssync/mozvcssync/github_pr.py:80 (Diff revision 1) > + if not os.path.exists(os.path.dirname(self.repo_path)): > + os.makedirs(os.path.dirname(self.repo_path)) > + logger.info('cloning %s to %s' % (fork_repo.clone_url, > + self.repo_path)) > + self.git('clone', fork_repo.clone_url, self.repo_path) > + os.chdir(self.repo_path) chdir is evil. Set the cwd argument of invoked processes instead. Alternatively, I believe you could also set GIT_WORK_TREE and GIT_DIR so Git uses explicit working directory and .git directories, respectively. ::: vcssync/mozvcssync/github_pr.py:83 (Diff revision 1) > + self.repo_path)) > + self.git('clone', fork_repo.clone_url, self.repo_path) > + os.chdir(self.repo_path) > + > + # setup remote upstream > + if 'upstream' not in self.git_output('remote'): Nit: this will fail in the edge case where `upstream` is a substring of another remote. Not that this will ever happen... ::: vcssync/mozvcssync/github_pr.py:106 (Diff revision 1) > + except: > + pass > + self.git('checkout', '-b', branch_name) > + > + # apply changes > + command = ['apply', patch_file] I'm nervous about the use of patch files. At the very least, the Mercurial side needs to ensure the correct `diff` config options are set or there could be data loss. The important one being git-style diffs. This is why I prefer to avoid patches completely and operate at the object level. Here, we could avoid checkouts completely and just write Git objects. ::: vcssync/mozvcssync/github_pr.py:107 (Diff revision 1) > + if include_paths is not None: > + for path in include_paths: for path in include_paths or [] (or do `include_paths = include_paths or []` at the top of the function) ::: vcssync/mozvcssync/github_pr.py:119 (Diff revision 1) > + temp_file = tempfile.NamedTemporaryFile(delete=False) > + try: You can use NamedTemporaryFile as a context manager. Just remember to .flush() the file handle so other processes can see data. ::: vcssync/mozvcssync/github_pr.py:136 (Diff revision 1) > + > + # insert user and token into push url > + u = urlparse.urlsplit(fork_repo.clone_url) > + url = re.sub(r'^//', '', > + urlparse.urlunsplit(['', u.netloc, u.path, u.query, ''])) > + push_auth_url = 'https://%s:%s@%s' % (urllib.quote(self.user), Must we put a private token in the URL? Surely there's a way we can configure Git to answer with that token for certain URLs? ::: vcssync/mozvcssync/servo_backout.py:31 (Diff revision 1) > +MOZREVIEW_COMMIT_ID_RE = re.compile( > + r'^MozReview-Commit-ID: .+$', > + re.MULTILINE > +) I'm pretty sure we have code for this in mozautomation. ::: vcssync/mozvcssync/servo_backout.py:46 (Diff revision 1) > + # Initialise integration repo. > + if not os.path.exists(integration_repo_path): > + logger.warn('%s does not exist; cloning %s' % (integration_repo_path, > + integration_repo_url)) > + subprocess.check_call([hglib.HGPATH, b'clone', b'--noupdate', > + b'--pull', integration_repo_url, Why did you add --pull here? For local testing? ::: vcssync/mozvcssync/servo_backout.py:61 (Diff revision 1) > + 'specified' % tracking_file) > + with open(tracking_file) as f: > + starting_revision = f.read().strip() > + > + # Iterate over each revision > + os.chdir(integration_repo_path) Why do you chdir here? hglib should work without it. ::: vcssync/mozvcssync/servo_backout.py:65 (Diff revision 1) > + # Iterate over each revision > + os.chdir(integration_repo_path) > + github_pr = None > + processed_revision = None > + with hglib.open(integration_repo_path, 'utf-8') as hg_repo: > + for commit in hg_repo.log('%s:' % starting_revision, files=['servo/']): This wants the :: operator, which follows the DAG (unlike : which follows storage order). ::: vcssync/mozvcssync/servo_backout.py:66 (Diff revision 1) > + node = commit[0] > + author = commit[4] > + desc = commit[5] Does hglib not return a named tuple? Boo if not. ::: vcssync/mozvcssync/servo_backout.py:87 (Diff revision 1) > + raise Exception('failed to find merge id in %s' > + % backed_out_node) I'm not sure what your plans are for dealing with this, but since this error is fatal and there's no way to advanced the last processed changeset, this will permanently break the backout service. As long as every changeset touching servo/ is a well-formed merge or backout, this won't come into play. Can we guarantee that? ::: vcssync/mozvcssync/servo_backout.py:91 (Diff revision 1) > + backed_out_url = '%s/pull/%s' % (github_pr.upstream_repo().html_url, > + backed_out_pr) This /may/ fail... because if the source repo for the PR has been deleted, I believe GitHub's API stops reporting certain details about it. I can't remember specifics. But this is something we may want to test. ::: vcssync/mozvcssync/servo_backout.py:127 (Diff revision 1) > + > + # PR created, mark tip as processed > + processed_revision = hg_repo.log(limit=1)[0][0] > + > + if processed_revision: > + with open(tracking_file, b'w') as f: Nit: you shouldn't need the b'' here. ::: vcssync/mozvcssync/servo_backout.py:130 (Diff revision 1) > + > + if processed_revision: > + with open(tracking_file, b'w') as f: > + f.write(b'%s\n' % processed_revision) > + > + # TODO so hacky. Relies on credentials in the environment. heh. At some point we'll need to do this properly.
Assignee | ||
Comment 13•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review145210 > Why did you add --pull here? For local testing? it's there because the current code applies patches. > Why do you chdir here? hglib should work without it. i definitely hit a command that didn't work without chdir at some point during development (`hg files` maybe, don't recall exactly). will check if it's still needed. > I'm not sure what your plans are for dealing with this, but since this error is fatal and there's no way to advanced the last processed changeset, this will permanently break the backout service. > > As long as every changeset touching servo/ is a well-formed merge or backout, this won't come into play. Can we guarantee that? every changeset should be one or the other, however as long as we allow non-automation to touch servo/ we can't guarentee. however this is only used for information purposes - so the original PR is linked to the backout. it should be safe to change this to a warning. > This /may/ fail... because if the source repo for the PR has been deleted, I believe GitHub's API stops reporting certain details about it. I can't remember specifics. But this is something we may want to test. the only place this is used is in the commit description of the backout; if it's no longer valid there's no harm. > Nit: you shouldn't need the b'' here. pycharm's typechecker inspection throws a warning without it; unicode_literals is enabled, and open() expects a str.
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review145210 > i definitely hit a command that didn't work without chdir at some point during development (`hg files` maybe, don't recall exactly). will check if it's still needed. it's still needed. here's an example of the command: ~$ hg log -R ~/Desktop/servo-pr/autoland -r '3f985817021e::' --template '{rev}\n' servo/ abort: servo/ not under root '/Users/byron/Desktop/servo-pr/autoland' (consider using '--cwd Desktop/servo-pr/autoland') servo/ is a subdir under the specified root path. hg's output suggests using --cwd, however that doesn't appear to be available when using the command server. this command works when cwd is set: ~/Desktop/servo-pr/autoland$ hg log -R ~/Desktop/servo-pr/autoland -r '3f985817021e::' --template '{rev}\n' servo/ 358354 358366 358375 ...
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review145210 > Must we put a private token in the URL? Surely there's a way we can configure Git to answer with that token for certain URLs? the other viable method is to switch to ssh and use cert auth (~/.netrc works, but that's a single per-user file not per-repo). as a token is already configured and deployed for linearize this is the path of least resistance and simplifies the setup should anyone else consume this lib.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8868027 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 35•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review145210 > Actually, we may just want to pass a github client instance into the function, as reusing clients can result in perf wins, so that's a behavior we want to encourage. as per our chat a while ago we need the token in order to authenticate this would mean consumers would have to provide both the token and a github3 object. i'll drop this, but keep this in mind if github login ends up being slower than expected.
Blocks: stylo-nightly-build
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 62•7 years ago
|
||
mozreview-review |
Comment on attachment 8868024 [details] servo-vcs-sync: create the wheelhouse in a shared location (bug 1365181) https://reviewboard.mozilla.org/r/139602/#review148836
Attachment #8868024 -
Flags: review?(gps) → review+
Comment 63•7 years ago
|
||
mozreview-review |
Comment on attachment 8873474 [details] testing: add hgext to vcssync and hgdev test suite (bug 1365181) https://reviewboard.mozilla.org/r/144848/#review148840 ::: testing/vcttesting/testing.py:45 (Diff revision 1) > + 'hgext': { > + 'venvs': {'hgdev', 'vcssync',}, > + }, First, this dict is an alphabetical order. More importantly, this dict defines directories containing Python unit tests to be executed with nose. I'm pretty sure that all tests in hgext/ are executed with Mercurial's test harness, not nose.
Attachment #8873474 -
Flags: review?(gps) → review-
Comment 64•7 years ago
|
||
mozreview-review |
Comment on attachment 8868025 [details] servo-vcs-sync: add mozautomation dependancy (bug 1365181) https://reviewboard.mozilla.org/r/139604/#review148842 ::: ansible/roles/vcs-sync/defaults/main.yml:3 (Diff revision 5) > --- > mozvcssync_wheel: mozvcssync-0.1-py2-none-any.whl > +mozautomation_wheel: mozautomation-0.2-py2-none-any.whl The version number here is a bit dangerous because the following can happen: 1. Deploy current code, which generates a 0.2 wheel 2. Bump version to 0.3 in setup.py 3. Generate 0.3 wheel 4. Deploy picks up 0.2 wheel because we forgot to update this version Can we get a comment in setup.py alerting us that we need to update this reference when the version is bumped? Alternatively, we could potential do some version number trickery to avoid the multiple references. But that's scope bloat.
Attachment #8868025 -
Flags: review?(gps) → review+
Comment 65•7 years ago
|
||
mozreview-review |
Comment on attachment 8868026 [details] servo-vcs-sync: add configuration for servo-backout-pr (bug 1365181) https://reviewboard.mozilla.org/r/139606/#review148844
Attachment #8868026 -
Flags: review+
Comment 66•7 years ago
|
||
mozreview-review |
Comment on attachment 8870458 [details] servo-vcs-sync: add extra_data param to pulse consumers (bug 1365181) https://reviewboard.mozilla.org/r/141898/#review148850
Attachment #8870458 -
Flags: review?(gps) → review+
Comment 67•7 years ago
|
||
mozreview-review |
Comment on attachment 8870459 [details] servo-vcs-sync: pass config object as extra_data to pulse consumers (bug 1365181) https://reviewboard.mozilla.org/r/141900/#review148852
Attachment #8870459 -
Flags: review?(gps) → review+
Comment 68•7 years ago
|
||
mozreview-review |
Comment on attachment 8870460 [details] servo-vcs-sync: move pulse callbacks to global functions (bug 1365181) https://reviewboard.mozilla.org/r/141902/#review148854
Attachment #8870460 -
Flags: review?(gps) → review+
Comment 69•7 years ago
|
||
mozreview-review |
Comment on attachment 8870461 [details] servo-vcs-sync: extract hg_converted handling into own method (bug 1365181) https://reviewboard.mozilla.org/r/141904/#review148856 ::: vcssync/mozvcssync/servo.py:83 (Diff revision 4) > if repo_url != config['hg_converted']: > + on_hg_converted(body) Calling the function when the current repo isn't the hg converted repo?
Attachment #8870461 -
Flags: review?(gps) → review-
Comment 70•7 years ago
|
||
mozreview-review |
Comment on attachment 8872893 [details] servo-vcs-sync: add servo-backout-pr services (bug 1365181) https://reviewboard.mozilla.org/r/144422/#review148858
Attachment #8872893 -
Flags: review?(gps) → review+
Comment 71•7 years ago
|
||
mozreview-review |
Comment on attachment 8873475 [details] overlay: add test for revsets (bug 1365181) https://reviewboard.mozilla.org/r/144850/#review148860 This isn't a great test because it isn't testing incremental overlay, which is the scenario we're using the feature for. But it's better than nothing. So r+.
Attachment #8873475 -
Flags: review?(gps) → review+
Comment 72•7 years ago
|
||
mozreview-review |
Comment on attachment 8873431 [details] servo-vcs-sync: add --source_revs to overlay-hg-repos (bug 1365181) https://reviewboard.mozilla.org/r/144840/#review148862
Attachment #8873431 -
Flags: review?(gps) → review+
Comment 73•7 years ago
|
||
mozreview-review |
Comment on attachment 8873432 [details] servo-vcs-sync: set source_revs to the backout author (bug 1365181) https://reviewboard.mozilla.org/r/144842/#review148864
Attachment #8873432 -
Flags: review?(gps) → review+
Comment 74•7 years ago
|
||
mozreview-review |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review149438 This generally looks pretty good! I need to spend some more time on the Git commands. But this certainly looks functional and in line with what we (mostly you) devised. ::: vcssync/mozvcssync/cli.py:301 (Diff revision 5) > + logging.getLogger('mozvcssync.servo_backout').setLevel(logging.INFO) > + logging.getLogger('mozvcssync.github_pr').setLevel(logging.INFO) Can we not set the level universally for all of `mozvcssync`? ::: vcssync/mozvcssync/github_pr.py:1 (Diff revision 5) > +from __future__ import unicode_literals Missing license header. ::: vcssync/mozvcssync/github_pr.py:16 (Diff revision 5) > +class GitCommand(object): > + """Helper class for running git commands""" > + def __init__(self, repo_path, token=None): > + self.repo_path = repo_path > + self.token = token > + > + def cmd(self, *command): > + command = command[0] if isinstance(command[0], list) else list(command) > + command = ['git', '-C', self.repo_path] + command > + command_str = ' '.join(command) > + if self.token: > + command_str = command_str.replace(self.token, 'xxx') > + logger.info('$ %s' % command_str) > + subprocess.check_call(command) > + > + def get(self, *command): > + command = command[0] if isinstance(command[0], list) else list(command) > + command = ['git', '-C', self.repo_path] + command > + return subprocess.check_output(command) This should go in gitutil.py. ::: vcssync/mozvcssync/github_pr.py:24 (Diff revision 5) > + self.repo_path = repo_path > + self.token = token > + > + def cmd(self, *command): > + command = command[0] if isinstance(command[0], list) else list(command) > + command = ['git', '-C', self.repo_path] + command It's probably best to execute the process with cwd set instead of using -C. ::: vcssync/mozvcssync/github_pr.py:77 (Diff revision 5) > + if self._upstream_repo is None: > + raise Exception('failed to find github repo: %s/%s' > + % (self.upstream_user, self.repo_name)) > + return self._upstream_repo > + > + def create_pr(self, Let's rename this create_pr_from_patch. ::: vcssync/mozvcssync/github_pr.py:128 (Diff revision 5) > + # Setup remote upstream. > + if 'upstream' not in git.get('remote').splitlines(): > + git.cmd('remote', 'add', 'upstream', upstream_repo.clone_url) This isn't strictly necessary. You can do something like `git fetch <URL> +refs/heads/master:refs/upstream/master` to fetch a specific ref from a URL and map it to a local ref. This avoids complexity for managing remotes. It also avoids fetching refs not relevant to the operation at hand. In some of the Git code I'm writing for pull request ingest, I actually generate a UUID and use that as the ref name because the actual names don't mean anything: they are just human-friendly labels (that also exist for garbage collection reasons). ::: vcssync/mozvcssync/github_pr.py:134 (Diff revision 5) > + if 'upstream' not in git.get('remote').splitlines(): > + git.cmd('remote', 'add', 'upstream', upstream_repo.clone_url) > + > + # reset > + git.cmd('checkout', 'master') > + git.cmd('reset', '--hard', 'origin/master') `origin` here assumes the original clone `origin` is the same as the current one, which may not always be true. ::: vcssync/mozvcssync/github_pr.py:149 (Diff revision 5) > + if not reuse_branch and git.get('branch', '--list', branch_name): > + git.cmd('branch', '--delete', '--force', branch_name) > + # noinspection PyBroadException > + try: > + git.cmd('push', 'origin', '--delete', branch_name) > + except: Why bareword except? Do you need to swallow KeyboardInterrupt and SystemExit? 99% of the time you want `except Exception`. ::: vcssync/mozvcssync/servo.py:97 (Diff revision 5) > + """Trigger backout service when a commit is backed out of the integration > + repo.""" Strictly speaking, this won't just fire for just backouts: it fires for all changes to the repo. It's still fine to trigger the service: it just no-ops if nothing to do. But the comments should be clear what's going on here. ::: vcssync/mozvcssync/servo_backout.py:134 (Diff revision 5) > + 'specified' % tracking_file) > + with open(tracking_file) as f: > + starting_revision = f.read().strip() > + # As the tracking file stores the last processed revision, we need > + # work on its children. > + starting_revision = 'children(%s)' % starting_revision Are you asserting there is only a single child? If so, that assertion may not be true. ::: vcssync/mozvcssync/servo_backout.py:139 (Diff revision 5) > + starting_revision = 'children(%s)' % starting_revision > + > + configs = ['ui.interactive=False'] > + with hglib.open(integration_repo_path, 'utf-8', configs) as hg_repo: > + # Update repo. > + hg_repo.update() Always pass a revision to update, even if it is `default` or `tip`. Semantics for the default update target are not static and have changed in Mercurial versions. Note: this may require you to look up the revision on the remote. You can use `hg -R <URL> identify` for that IIRC. ::: vcssync/mozvcssync/servo_backout.py:161 (Diff revision 5) > + for backed_out_node in nodes: > + pr_url = _create_pr(integration_repo_path, hg_repo, github_pr, > + backed_out_node, commit.desc, commit.node, > + author) So, this means if we're backing out multiple commits that we'll create the PR then update it N times in rapid order. This works. But there may be consequences, such as entities listening for PRs getting overwhelmed (e.g. scheduling multiple runs of automation against each PR update). ::: vcssync/mozvcssync/servo_backout.py:172 (Diff revision 5) > + pr_url)) > + > + # PRs created, mark tip as processed. > + processed_revision = hg_repo.log(limit=1)[0][0] > + > + if processed_revision: We currently ignore commits that don't have a backout. So in the common case where there is a push that isn't a servo backout, we never advance the last processed revision. This leads to redundant work. I think we should advance the pointer so it is always current.
Attachment #8868028 -
Flags: review?(gps) → review-
Assignee | ||
Comment 75•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8873474 [details] testing: add hgext to vcssync and hgdev test suite (bug 1365181) https://reviewboard.mozilla.org/r/144848/#review148840 > First, this dict is an alphabetical order. > > More importantly, this dict defines directories containing Python unit tests to be executed with nose. I'm pretty sure that all tests in hgext/ are executed with Mercurial's test harness, not nose. get_test_files() uses UNIT_TEST_DIRS to find both types of tests; populating both unit_test (test*.py) and hook_tests (test*.t).
Assignee | ||
Comment 76•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review149438 > We currently ignore commits that don't have a backout. So in the common case where there is a push that isn't a servo backout, we never advance the last processed revision. This leads to redundant work. > > I think we should advance the pointer so it is always current. `processed_revision` is always set even if no backouts were found. i'll remove the redundant and misleading `if processed_revision`.
Assignee | ||
Comment 77•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review149438 > `origin` here assumes the original clone `origin` is the same as the current one, which may not always be true. i put some time against this, but it looks like it's trickier that i expected assuming we don't want to get into the "managing remotes" game. this isn't a problem for our automation, so i'll drop this and add a comment that if the origin is changed after cloning things may break. > Are you asserting there is only a single child? If so, that assertion may not be true. within the context of servo backouts this shouldn't be a problem - we're only supporting backouts on the autoland repo.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Attachment #8873431 -
Attachment is obsolete: true
Comment 91•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8873474 [details] testing: add hgext to vcssync and hgdev test suite (bug 1365181) https://reviewboard.mozilla.org/r/144848/#review148840 > get_test_files() uses UNIT_TEST_DIRS to find both types of tests; populating both unit_test (test*.py) and hook_tests (test*.t). Please read https://hg.mozilla.org/hgcustom/version-control-tools/rev/d1f6cc40b04b and let me know if things make more sense.
Comment 92•7 years ago
|
||
mozreview-review |
Comment on attachment 8870461 [details] servo-vcs-sync: extract hg_converted handling into own method (bug 1365181) https://reviewboard.mozilla.org/r/141904/#review153122
Attachment #8870461 -
Flags: review?(gps) → review+
Comment 93•7 years ago
|
||
mozreview-review |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153270 I haven't yet reviewed `github_pr.py`. I may or may not get to that tonight and wanted to make sure I left you with some review to act on. ::: vcssync/mozvcssync/gitutil.py:31 (Diff revision 6) > + self.repo_path = repo_path > + self.logger = logger > + self.secret = secret > + > + def cmd(self, *command): > + command = command[0] if isinstance(command[0], list) else list(command) This employs just enough magic to warrant a comment, preferably in a docstring. Unless you need to use `*args`, I'd just accept a single argument which is an iterable of command arguments. Although I do concede that `*args` makes code more concise at the call site. ::: vcssync/mozvcssync/gitutil.py:34 (Diff revision 6) > + > + def cmd(self, *command): > + command = command[0] if isinstance(command[0], list) else list(command) > + command = ['git'] + command > + if self.logger: > + command_str = ' '.join(command) `command_str = ' '.join(map(pipes.quote, command))` If you do that, it should be possible to copy and paste the command, even if arguments contain whitespace, etc. ::: vcssync/mozvcssync/servo_backout.py:46 (Diff revision 6) > + r'^servo: Merge #([0-9]+)' > +) > + > + > +def _find_backout_commits(hg_repo, starting_revision): > + LightWeightCommit = namedtuple('LightWeightCommit', 'node desc author') TIL the 2nd value accepts a space delimited list of fields. Can you change that to a tuple? Also, please move the type assignment outside of the function, otherwise you create a type on every invocation and the types will actually be different! I don't think this really matters in this use case. But if it ever does, you don't want to track down that bug :) ::: vcssync/mozvcssync/servo_backout.py:48 (Diff revision 6) > + > + > +def _find_backout_commits(hg_repo, starting_revision): > + LightWeightCommit = namedtuple('LightWeightCommit', 'node desc author') > + commits = [] > + for commit in hg_repo.log('%s::' % starting_revision, Per video conversation, this wants a `- merge()` because we never care about merges. ::: vcssync/mozvcssync/servo_backout.py:52 (Diff revision 6) > + commit.desc.decode('utf-8'), > + commit.author.decode('utf-8'))) Why the convert to Unicode? Can't we keep as bytes? ::: vcssync/mozvcssync/servo_backout.py:61 (Diff revision 6) > + return commits > + > + > +def _find_backed_out_urls(hg_repo, github_url, commit_desc, commit_node): > + backed_out_urls = [] > + revs, bugs = commitparser.parse_backouts(commit_desc) We do this parsing twice. Do you think it is worth storing result from `_find_backout_commits` in the returned data structure? ::: vcssync/mozvcssync/servo_backout.py:91 (Diff revision 6) > + assert filename.startswith('servo/') > + return filename[len('servo/'):] > + > + # Grab a list of files under the servo/ directory touched by this > + # change. > + args = cmdbuilder('log', template='{join(files,"\n")}', Can `\0` be used as a separator? ::: vcssync/mozvcssync/servo_backout.py:115 (Diff revision 6) > + if len(github_repo_name.split('/')) != 2: > + raise Exception('"%s" is not in the form "user/repo"' > + % github_repo_name) You could do `user, repo = github_repo_name.split('/')` and count on that to raise an exception if it doesn't split into exactly 2 pieces. ::: vcssync/mozvcssync/servo_backout.py:134 (Diff revision 6) > + if not starting_revision: > + # We never want to walk all commits. > + if not os.path.exists(tracking_file): > + raise Exception('%s does not exist, and a revision was not ' > + 'specified' % tracking_file) > + with open(tracking_file) as f: I always forget what the default mode to `open()` is. So please specify it. And make the mode `rb`, which is compatible with Python 3. (`r` in Python 3 opens in text mode which involves Unicode - eww.) ::: vcssync/mozvcssync/servo_backout.py:140 (Diff revision 6) > + starting_revision = f.read().strip() > + # As the tracking file stores the last processed revision, we need > + # work on its children. > + # This assumes that backouts to servo files will only happen on the > + # integration/autoland repository. > + starting_revision = 'children(%s)' % starting_revision Per video chat, I think I convinced you to use `{{rev}}:: - {{rev}}`. ::: vcssync/mozvcssync/servo_backout.py:142 (Diff revision 6) > + # work on its children. > + # This assumes that backouts to servo files will only happen on the > + # integration/autoland repository. > + starting_revision = 'children(%s)' % starting_revision > + > + configs = ['ui.interactive=False'] Out of curiosity, what prompted you to add this? ::: vcssync/mozvcssync/servo_backout.py:147 (Diff revision 6) > + configs = ['ui.interactive=False'] > + with hglib.open(integration_repo_path, 'utf-8', configs) as hg_repo: > + # Update repo if required. > + local_tip = hg_repo.identify(id=True).strip() > + remote_tip = subprocess.check_output([ > + hglib.HGPATH, 'identify', integration_repo_url]).strip() Please humor me by adding `-r tip` to this. ::: vcssync/mozvcssync/servo_backout.py:150 (Diff revision 6) > + local_tip = hg_repo.identify(id=True).strip() > + remote_tip = subprocess.check_output([ > + hglib.HGPATH, 'identify', integration_repo_url]).strip() > + if local_tip != remote_tip: > + logger.info('updating %s to %s' % (integration_repo_path, remote_tip)) > + hg_repo.pull(source=integration_repo_url) Since we only need to use remote tip, let's only pull that revision. ::: vcssync/mozvcssync/servo_backout.py:156 (Diff revision 6) > + if backout_commits: > + github_pr = GitHubPR(github_token, github_repo_name, > + github_repo_path) Consider pulling this block into a separate function to aid readability of this long function. ::: vcssync/mozvcssync/servo_backout.py:182 (Diff revision 6) > + # Checkout the backout commit to ensure we're copying the > + # correct file contents. > + hg_repo.update(rev=commit.node) Somewhere in here we'll also need to purge/clean the working directory. Call `clean_hg_repo()` in util.py. ::: vcssync/mozvcssync/servo_backout.py:193 (Diff revision 6) > + if not os.path.exists(source_file): > + logger.info('removing %s' % filename) > + try: > + os.unlink(dest_file) > + except OSError as e: > + # ignore FileNotFound > + if e.errno != errno.ENOENT: > + raise > + else: > + logger.info('updating %s' % filename) > + shutil.copy(source_file, dest_file) I feel like there is a bug involving deleted/added files here. I need to think about this some more. Keep in mind that the `{files}` returned by `hg log` is a union of all modifies, adds, deletes, copies, and renames (copies and deletes being special cases of adds and deletes). Please be sure to write tests covering file add, remove, copy, and rename scenarios. ::: vcssync/mozvcssync/servo_backout.py:215 (Diff revision 6) > + # PRs created, mark tip as processed. > + processed_revision = hg_repo.log(limit=1)[0].node We never commit to the hg repo. So I think it is semantically better to set this to `remote_tip`. If so, do we need this variable at all? ::: vcssync/mozvcssync/servo_backout.py:219 (Diff revision 6) > + > + # PRs created, mark tip as processed. > + processed_revision = hg_repo.log(limit=1)[0].node > + > + if processed_revision != starting_revision: > + with open(tracking_file, b'w') as f: `'wb'` You don't need to pass bytes to the mode argument. And I think this will actually fail on Python 3!
Attachment #8868028 -
Flags: review?(gps)
Assignee | ||
Comment 94•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8873474 [details] testing: add hgext to vcssync and hgdev test suite (bug 1365181) https://reviewboard.mozilla.org/r/144848/#review148840 > Please read https://hg.mozilla.org/hgcustom/version-control-tools/rev/d1f6cc40b04b and let me know if things make more sense. you said "it isn't necessary to list directories here that are scanned for Mercurial tests". this isn't correct. get_test_files() uses UNIT_TEST_DIRS *only* to find tests, and with the vcssync env active, hgext currently isn't scanned for tests. i want to be able to run hgext tests with the vcssync venv active using run-tests. this isn't possible without this change. unpatched: $ ./run-tests hgext/overlay/tests/test-overlay-basic.t ...... this is running the whole test suite, which does not include hgext/overlay/tests patched: $ ./run-tests hgext/overlay/tests/test-overlay-basic.t . (runs just the test i requested)
Comment 95•7 years ago
|
||
mozreview-review |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153300 Still need to look at this in more detail. My eyes are starting to glaze over because I'm tired. But some useful feedback before I sleep. ::: vcssync/mozvcssync/github_pr.py:36 (Diff revision 6) > + self.github = github or github3.GitHub() > + self.github = github3.login(token=self.token) Eh? ::: vcssync/mozvcssync/github_pr.py:150 (Diff revision 6) > + git.cmd('checkout', 'master') > + # Note this assumes the origin remote hasn't been changed since the > + # initial clone. > + git.cmd('reset', '--hard', 'origin/master') This does 2 checkouts. First, it updates the working directory to the `master` branch then it forcefully resets `master` to `origin/master` and updates the working directory to match. This could be quite expensive. Try running `git update-ref` to forcefully set local `master` to `origin/master` then do `git checkout master` to do a working directory update. ::: vcssync/mozvcssync/github_pr.py:157 (Diff revision 6) > + # initial clone. > + git.cmd('reset', '--hard', 'origin/master') > + git.cmd('clean', '-d', '--force') > + > + # update master > + git.cmd('fetch', upstream_repo.clone_url, 'master:refs/upstream/master') The refspec should be prefixed with `+` to allow non-fast-forward changes. In other words, allow any changes on the remote to be reflected to the local ref. Basically a force fetch. ::: vcssync/mozvcssync/github_pr.py:192 (Diff revision 6) > + > + if git.get('status', '--short'): > + > + # Commit changes. > + with tempfile.NamedTemporaryFile() as temp_file: > + temp_file.write(description.encode('utf-8')) Not sure why this variable is unicode.
Assignee | ||
Comment 96•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153270 > Why the convert to Unicode? Can't we keep as bytes? needs to be unicode so it can be treated correctly later on (eg. when writing to a file for the git commit desc). > We do this parsing twice. Do you think it is worth storing result from `_find_backout_commits` in the returned data structure? i don't think it's worth doing that. the performance of parse_backouts() is pretty good, keeping these split makes testing easier, and it would be clumsy to do avoid the duplicate call because parse_backouts() returns None when there's no match. > Can `\0` be used as a separator? it could, but i don't see the value. \n makes this easier to debug, and filenames won't contain \n. > You could do `user, repo = github_repo_name.split('/')` and count on that to raise an exception if it doesn't split into exactly 2 pieces. i think this comes down to style. i prefer to avoid using exceptions when a simple comparison will do. doubly so in this case where we just do validation here; the split values are used in another class. > Per video chat, I think I convinced you to use `{{rev}}:: - {{rev}}`. i can't use that exact syntax: $ hg log -r '6010f4b0f4fd:: - 6010f4b0f4fd' abort: unknown revision '-6010f4b0f4fd'! looks like it's parsing the negation as a revision. explicitly defining the default value fort the second part of the dag range works: hg log -r '6010f4b0f4fd::descendants(6010f4b0f4fd) - 6010f4b0f4fd' > Out of curiosity, what prompted you to add this? to guard against hg blocking waiting for stdin. this should never happen with the current commands, but i see value in raising an error should that happen instead of blocking. > `'wb'` > > You don't need to pass bytes to the mode argument. And I think this will actually fail on Python 3! passing in str here is to keep linting happy; it's complaining that the definition for open() is (unicode, str), while i'm passing in (unicode, unicode) due to unicode_literals. you're right that this will fail on py3, so i'll change to 'wb'. i'll add a `noinspection PyTypeChecker` override to this and other open statements to keep pycharm happy.
Assignee | ||
Comment 97•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153300 > Eh? this makes testing with betamax possible. will add a comment.
Assignee | ||
Comment 98•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153270 > Somewhere in here we'll also need to purge/clean the working directory. Call `clean_hg_repo()` in util.py. will do, but to be honest i'm not sure why it's required as we don't make changes to the hg repo.
Comment 99•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153270 > i can't use that exact syntax: > > $ hg log -r '6010f4b0f4fd:: - 6010f4b0f4fd' > abort: unknown revision '-6010f4b0f4fd'! > > looks like it's parsing the negation as a revision. explicitly defining the default value fort the second part of the dag range works: > > hg log -r '6010f4b0f4fd::descendants(6010f4b0f4fd) - 6010f4b0f4fd' Um, I thought you were attempting to identify a single revision with this (Since there will only ever be a single child)? The new revset will identify all descendants of the revision. In fact, `hg log -r '6010f4b0f4fd::descendants(6010f4b0f4fd) - 6010f4b0f4fd'` should be the exact same as `hg log -r 'descendants(6010f4b0f4fd)'` (well, order *might* be different...)
Comment 100•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153300 > this makes testing with betamax possible. will add a comment. I think gps' is confused because there is a botched merge here with my changes. this should look something like: ``` self.github = github or github3.GitHub() self.github.login(token=self.token) ``` you're calling login on `github3`, not `github`. The tests should fail with the state you currently have.
Assignee | ||
Comment 101•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153300 > I think gps' is confused because there is a botched merge here with my changes. this should look something like: > ``` > self.github = github or github3.GitHub() > self.github.login(token=self.token) > ``` > > you're calling login on `github3`, not `github`. The tests should fail with the state you currently have. oops; well spotted. fixed.
Comment 102•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153270 > This employs just enough magic to warrant a comment, preferably in a docstring. Unless you need to use `*args`, I'd just accept a single argument which is an iterable of command arguments. Although I do concede that `*args` makes code more concise at the call site. Why not just have callers use `cmd(*im_a_list)` when they call `cmd()` with a pre-existing list?
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 119•7 years ago
|
||
mozreview-review |
Comment on attachment 8878022 [details] servo-vcs-sync: add basic testing for apply_changes_from_list (bug 1365181) https://reviewboard.mozilla.org/r/149436/#review154050 ::: vcssync/mozvcssync/util.py:170 (Diff revision 1) > +if __name__ == '__main__': > + if not sys.argv: > + sys.exit() > + > + if sys.argv[1] == 'apply_changes_from_list': > + logger = logging.getLogger('mozvcssync') > + logger.addHandler(logging.StreamHandler(sys.stdout)) > + logger.setLevel(logging.INFO) > + source_path, dest_path, filelist = sys.argv[2::] > + apply_changes_from_list(logger, > + source_path, dest_path, filelist.split(',')) yes, this is eww. it's just something quick and dirty so i can push up the rest of the changes along with the simple test this supports. i'll rework this to move it to a test-only cli script.
Assignee | ||
Comment 120•7 years ago
|
||
mozreview-review |
Comment on attachment 8876807 [details] vcssync: install nose in test environment (bug 1365181) https://reviewboard.mozilla.org/r/148130/#review154286
Attachment #8876807 -
Flags: review?(glob) → review+
Comment 121•7 years ago
|
||
mozreview-review |
Comment on attachment 8878022 [details] servo-vcs-sync: add basic testing for apply_changes_from_list (bug 1365181) https://reviewboard.mozilla.org/r/149436/#review155114 ::: commit-message-16e76:5 (Diff revision 1) > +This adds a simple test for apply_changes_from_list. As this function relies > +on a filesystem to determine what actions to perform I ended up using run-tests > +with a simple wrapper in util.py over unit/nosetests. unit/nosetests are also integrated with run-tests, so it's more accurate to say you ended up using a ".t" test. (The other piece, is it's trivial to create a temporary directory in if you wanted to use a unit test. I wouldn't be surprised if run-tests is already passing a temp directory or starting the test inside one anyway. This .t test is fine though, and the shell for creating the dirs and files is very readable). ::: vcssync/tests/test-backout-sync-changes.t:2 (Diff revision 1) > + $ . $TESTDIR/vcssync/tests/helpers.sh > + $ alias t="python $TESTDIR/vcssync/mozvcssync/util.py apply_changes_from_list $TESTTMP/src $TESTTMP/dst" I'd actually prefer something more verbose here, such as "apply_changes" - it'd make it easier for me to just visually pick out where you're calling the alias, and remind me what we're actually doing if I just look at a portion below. ::: vcssync/tests/test-backout-sync-changes.t:4 (Diff revision 1) > + $ . $TESTDIR/vcssync/tests/helpers.sh > + $ alias t="python $TESTDIR/vcssync/mozvcssync/util.py apply_changes_from_list $TESTTMP/src $TESTTMP/dst" > + > + $ mkdir -p $TESTTMP/src/sub isn't the working directory already a temporary directory? I think it'd be more readable without $TESTTMP scattered all over.
Attachment #8878022 -
Flags: review?(smacleod) → review+
Comment 122•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8878022 [details] servo-vcs-sync: add basic testing for apply_changes_from_list (bug 1365181) https://reviewboard.mozilla.org/r/149436/#review154050 > yes, this is eww. > > it's just something quick and dirty so i can push up the rest of the changes along with the simple test this supports. > > i'll rework this to move it to a test-only cli script. It's actually *less* code to make this a command line script created by the entry points. You'd just make everything below `line 174` part of a function, and then just add that function to the entry points. I think it's a lot cleaner to do that than have this strange util file which is also executable. That being said, you're planning to update this soon anyway, so I won't push for this.
Assignee | ||
Comment 123•7 years ago
|
||
mozreview-review |
Comment on attachment 8876808 [details] servo-vcs-sync: add basic unit testing for github_pr (bug 1365181) https://reviewboard.mozilla.org/r/148132/#review155156 lgtm
Attachment #8876808 -
Flags: review?(glob) → review+
Comment 124•7 years ago
|
||
mozreview-review |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review155264 ::: vcssync/mozvcssync/servo_backout.py:128 (Diff revision 7) > + author = commit.author > + > + desc = _build_commit_desc(commit.desc, backed_out_urls) > + > + # This incantation forces this PR to the head of homu's queue. > + pr_body = '@homu force treeclosed=9001\n\n%s' % desc I believe that this should be: `@bors-servo r+ force treeclosed=9001`
Comment 125•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8873474 [details] testing: add hgext to vcssync and hgdev test suite (bug 1365181) https://reviewboard.mozilla.org/r/144848/#review148840 > you said "it isn't necessary to list directories here that are scanned for Mercurial tests". > this isn't correct. get_test_files() uses UNIT_TEST_DIRS *only* to find tests, and with the vcssync env active, hgext currently isn't scanned for tests. > > i want to be able to run hgext tests with the vcssync venv active using run-tests. this isn't possible without this change. > > unpatched: > $ ./run-tests hgext/overlay/tests/test-overlay-basic.t > ...... > this is running the whole test suite, which does not include hgext/overlay/tests > > patched: > $ ./run-tests hgext/overlay/tests/test-overlay-basic.t > . > (runs just the test i requested) OK - I see the problem. `get_test_files()` only processes `hgext/` and `hghooks/` for an allow list of virtualenvs. For all virtualenvs, it processes `UNIT_TEST_DIRS`. Since the vcssync virtualenv isn't in the allow list, you need to supplement with `UNIT_TEST_DIRS`. This **is** the correct thing to do. However, I still think the patch is buggy: I think you should only list `hgext/overlay` for the vcssync virtualenv and leave `hgext/` the domain of `get_test_files()`. Also, there's probably some code paths in `get_test_files()` that can result in redundant entries. We should probably record tests as a set instead of a list. But that's for another patch/series.
Comment 126•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review153270 > will do, but to be honest i'm not sure why it's required as we don't make changes to the hg repo. If a process dies mid update or the machine is powered down without flushing filesystem writes, the working directory could be in an undefined state. It is best to not take chances.
Comment 127•7 years ago
|
||
mozreview-review |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review155460 If you want to land this, go ahead. I can do a post review tomorrow.
Attachment #8868028 -
Flags: review?(gps) → review+
Comment 128•7 years ago
|
||
mozreview-review |
Comment on attachment 8873474 [details] testing: add hgext to vcssync and hgdev test suite (bug 1365181) https://reviewboard.mozilla.org/r/144848/#review155462 Granting r+ so you can autoland. Please do fix the lingering issue with being greedy about all of `hgext/`.
Attachment #8873474 -
Flags: review?(gps) → review+
Attachment #8868028 -
Flags: review?(smacleod)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Keywords: leave-open
Comment 145•7 years ago
|
||
Pushed by bjones@mozilla.com: https://hg.mozilla.org/hgcustom/version-control-tools/rev/69b4ecd96bec servo-vcs-sync: fix PEP8 warnings due to long lines r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/760cdce02edc servo-vcs-sync: create the wheelhouse in a shared location r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/3a111f471c95 testing: add hgext to vcssync and hgdev test suite r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/510eccf4bc5b servo-vcs-sync: add mozautomation dependancy r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/1554e1398447 servo-vcs-sync: add configuration for servo-backout-pr r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/25eab1192144 servo-vcs-sync: add extra_data param to pulse consumers r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/f52386fce1b3 servo-vcs-sync: pass config object as extra_data to pulse consumers r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/4d6b4e5f700b servo-vcs-sync: move pulse callbacks to global functions r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/e5a6f53e6079 servo-vcs-sync: extract hg_converted handling into own method r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/35e9f1dc4c04 servo-vcs-sync: create servo/servo pull requests from mercurial backouts r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/8e860aa2f36e servo-vcs-sync: add servo-backout-pr services r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/a49367f0d09b overlay: add test for revsets r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/60d7f201a1c3 servo-vcs-sync: set source_revs to the backout author r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/258f484c4901 vcssync: install nose in test environment r=glob https://hg.mozilla.org/hgcustom/version-control-tools/rev/e11241fb6da3 servo-vcs-sync: add basic unit testing for github_pr r=glob https://hg.mozilla.org/hgcustom/version-control-tools/rev/12c5a729847a servo-vcs-sync: add basic testing for apply_changes_from_list r=smacleod
Comment 146•7 years ago
|
||
mozreview-review |
Comment on attachment 8868028 [details] servo-vcs-sync: create servo/servo pull requests from mercurial backouts (bug 1365181) https://reviewboard.mozilla.org/r/139610/#review155646 As indicated over vidyo chat, I was comfortable with this landing and have looked over it several times without leaving an official review. Any other feedback I have can be handled as followups, looks good overall.
Attachment #8868028 -
Flags: review?(smacleod) → review+
Assignee | ||
Comment 147•7 years ago
|
||
this has been deployed; needs end-to-end testing before this bug can be marked as resolved/fixed. deployment required the following fixes: https://hg.mozilla.org/hgcustom/version-control-tools/rev/1ed16b7a74382924187ac8b490f86295c497ba37 https://hg.mozilla.org/hgcustom/version-control-tools/rev/42fe71b273500e5e77be3267156b5bb388f42486 https://hg.mozilla.org/hgcustom/version-control-tools/rev/9332f93eefc271e6b2f8b1198eb74f9156e60f23 https://hg.mozilla.org/hgcustom/version-control-tools/rev/f880b791791a32df156254b029c79f2dbf8292f0 https://hg.mozilla.org/hgcustom/version-control-tools/rev/5513f55edd73175156871f27f11b213d322e83e4 https://hg.mozilla.org/hgcustom/version-control-tools/rev/0b26b6a5a7dcf94b07f9e27259271aa12fcae9b0 https://hg.mozilla.org/hgcustom/version-control-tools/rev/11d71be760bfa258ac007245a8f97c919ed8fb2c https://hg.mozilla.org/hgcustom/version-control-tools/rev/6e92eba9939e8151e984fe17d96550b6723bd92e https://hg.mozilla.org/hgcustom/version-control-tools/rev/e079104ee147d5ad7537b32cd2a888d664dcb5d5 https://hg.mozilla.org/hgcustom/version-control-tools/rev/941b10f8656d0d555a9615f156787295a9f40fad
Assignee | ||
Comment 148•7 years ago
|
||
status update: this is looking good, however i'm waiting on bug 1374534 to be resolved before marking this as r/f. the changes there are required to allow recovery that should the two repositories get out of sync.
Assignee | ||
Comment 149•7 years ago
|
||
bug 1374534 has been fixed, and i've pushed docs for it https://hg.mozilla.org/hgcustom/version-control-tools/rev/bdf9cf901d9b4a9cdddfaa87b616fbbeb95b82d7 mozilla-version-control-tools.readthedocs.io/en/latest/vcssync/servo.html isn't updating, but that alone isn't good enough to delay marking this as fixed. i'll email the sheriff and stylo teams once the docs are up.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Comment 150•7 years ago
|
||
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in
before you can comment on or make changes to this bug.
Description
•