Closed
Bug 1093273
Opened 10 years ago
Closed 10 years ago
b2g bumper bot doesn't handle <remove-project> elements
Categories
(Release Engineering :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: catlee, Assigned: catlee)
Details
Attachments
(1 file, 1 obsolete file)
2.80 KB,
patch
|
pmoore
:
review+
catlee
:
checked-in+
|
Details | Diff | Splinter Review |
e.g.
https://github.com/mozilla-b2g/b2g-manifest/blob/master/base-kk-caf.xml#L7
we should support this, and probably raise warnings in case we encounter other unsupported elements.
Assignee | ||
Comment 1•10 years ago
|
||
The current state of things is that we completely ignore <remove-project> tags in the manifests. This has no ill effects for the builds as `repo` knows how to deal with them. The only impact of having them is slightly confusing manifests to read, and possibly having us do extra lookups and mirroring for populating the revisions for the removed projects.
This patch removes the referenced projects and the <remove-project> tag from the manifest. It should be a NOOP as far as builds are concerned...
Attachment #8550544 -
Flags: review?(pmoore)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → catlee
Comment 2•10 years ago
|
||
Let me test this in my fork of b2g-manifest...
It looks good from a visual inspection.
One question: we error if we find a tagName that we don't know, but we do allow unknown nodeTypes. Should we also error for unknown nodeTypes too, to be consistent?
Flags: needinfo?(catlee)
Comment 3•10 years ago
|
||
Currently testing b2g bumper in: https://travis-ci.org/petemoore/b2g-manifest/builds/47501766
Mozharness tests passed in: https://travis-ci.org/petemoore/build-mozharness/builds/47501709
Comment 4•10 years ago
|
||
(In reply to Pete Moore [:pete][:pmoore] from comment #2)
> Let me test this in my fork of b2g-manifest...
>
> It looks good from a visual inspection.
>
> One question: we error if we find a tagName that we don't know, but we do
> allow unknown nodeTypes. Should we also error for unknown nodeTypes too, to
> be consistent?
Whoops - I misread "if node.nodeType in" as "if node.nodeType not in" - so forget my question - I see we explicitly allow TEXT_NODE and COMMENT_NODE, and nothing else, so all ok. =)
Flags: needinfo?(catlee)
Comment 5•10 years ago
|
||
Comment on attachment 8550544 [details] [diff] [review]
support <remove-project> tags
Review of attachment 8550544 [details] [diff] [review]:
-----------------------------------------------------------------
r+ with fixes
::: mozharness/mozilla/repo_manifest.py
@@ +9,5 @@
> def load_manifest(filename):
> """
> Loads manifest from `filename`
> Processes any <include name="..." /> nodes
> + Abort on unsupported manifest tags
Can you add a line that projects with <remove-project name="..." /> are removed?
I noticed the include mechanism also isn't recursive, so includes of includes wouldn't get processed - do you think we should fix this too?
Lastly it might also be worth elaborating on the "Processes any <include ..." comment to highlight that a single flattened manifest is generated, with sub-includes in-place, and that the method returns the root node of the resulting DOM.
@@ +24,5 @@
> +
> + if node.tagName not in ('include', 'project', 'remote', 'default', 'manifest', 'copyfile', 'remove-project'):
> + raise ValueError("Unsupported tag: %s" % node.tagName)
> + to_visit.extend(node.childNodes)
> +
Can you move this whole section above to after the for loop below, so that the included manifests are also validated?
@@ +62,5 @@
> + to_remove.append(project_node)
> + to_remove.append(node)
> +
> + for r in to_remove:
> + print "REMOVING", r
Is it possible to use mozharness logging here, rather than print?
Attachment #8550544 -
Flags: review?(pmoore) → review+
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Pete Moore [:pete][:pmoore] from comment #5)
> Comment on attachment 8550544 [details] [diff] [review]
> support <remove-project> tags
>
> Review of attachment 8550544 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r+ with fixes
>
> ::: mozharness/mozilla/repo_manifest.py
> @@ +9,5 @@
> > def load_manifest(filename):
> > """
> > Loads manifest from `filename`
> > Processes any <include name="..." /> nodes
> > + Abort on unsupported manifest tags
>
> Can you add a line that projects with <remove-project name="..." /> are
> removed?
will do
>
> I noticed the include mechanism also isn't recursive, so includes of
> includes wouldn't get processed - do you think we should fix this too?
It is recursive; we call load_manifest on the referenced manifest:
>>> inc_doc = load_manifest(inc_filename).documentElement
so it should have its includes and remove-projects processed.
> Lastly it might also be worth elaborating on the "Processes any <include
> ..." comment to highlight that a single flattened manifest is generated,
> with sub-includes in-place, and that the method returns the root node of the
> resulting DOM.
>
> @@ +24,5 @@
> > +
> > + if node.tagName not in ('include', 'project', 'remote', 'default', 'manifest', 'copyfile', 'remove-project'):
> > + raise ValueError("Unsupported tag: %s" % node.tagName)
> > + to_visit.extend(node.childNodes)
> > +
>
> Can you move this whole section above to after the for loop below, so that
> the included manifests are also validated?
it should be handled by the recursive call
> @@ +62,5 @@
> > + to_remove.append(project_node)
> > + to_remove.append(node)
> > +
> > + for r in to_remove:
> > + print "REMOVING", r
>
> Is it possible to use mozharness logging here, rather than print?
no, we don't have access to mozharness logging objects here. I'll remove this output.
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8550544 -
Attachment is obsolete: true
Attachment #8551296 -
Flags: review?(pmoore)
Comment 8•10 years ago
|
||
(In reply to Chris AtLee [:catlee] from comment #6)
> (In reply to Pete Moore [:pete][:pmoore] from comment #5)
> > I noticed the include mechanism also isn't recursive, so includes of
> > includes wouldn't get processed - do you think we should fix this too?
>
> It is recursive; we call load_manifest on the referenced manifest:
>
> >>> inc_doc = load_manifest(inc_filename).documentElement
Whoops. How did I not see that? Sorry for the noise!
Updated•10 years ago
|
Attachment #8551296 -
Flags: review?(pmoore) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8551296 -
Flags: checked-in+
Comment 9•10 years ago
|
||
In production: https://hg.mozilla.org/build/mozharness/rev/e9ea07c7b917
Assignee | ||
Updated•10 years ago
|
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Component: General Automation → General
You need to log in
before you can comment on or make changes to this bug.
Description
•