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)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: catlee, Assigned: catlee)

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch support <remove-project> tags (obsolete) — Splinter Review
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: nobody → catlee
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)
(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 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+
(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.
Attachment #8550544 - Attachment is obsolete: true
Attachment #8551296 - Flags: review?(pmoore)
(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!
Attachment #8551296 - Flags: review?(pmoore) → review+
Attachment #8551296 - Flags: checked-in+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Component: General Automation → General
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: