874091 - address minor review comments on Servo mozharness scripts

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[[e-mail] (:bhearsum)]

From https://bugzilla.mozilla.org/show_bug.cgi?id=863277#c25

Comment on attachment 751052 [details] [diff] [review] [diff] [review]
mozharness scripts to build servo

>+config = {
>+    'mock_target': 'mozilla-centos6-x86_64',
>+    'mock_packages': ['freetype-devel', 'fontconfig-devel', 'glib2-devel', 'autoconf213', 'git', 'make', 'libX11-devel', 'mesa-libGL-devel', 'freeglut-devel'],
>+    'mock_files': [('/home/servobld/.ssh', '/home/mock_mozilla/.ssh')],
>+}

That's impressively sparse.
I'm going to guess Windows will be larger.

Since you do have a separate config file, it would be easy enough to have the script's default_actions be everything but setup-mock, and then override those default_actions in this sonfig file (with everything including setup-mock).
However, making setup-mock noop is also acceptable, as I said in IRC.

>@@ -0,0 +1,128 @@
>+#!/usr/bin/env python
>+# Mozilla licence shtuff

Hunh, dictionary.com takes either licence or license.
Anyway, to populate.

>+SUCCESS, WARNINGS, FAILURE, EXCEPTION = xrange(4)

This is fine and may be superior to the current levels in mozharness.mozilla.buildbot, but it might be nice to standardize.

>+class ServoBuild(MockMixin, BaseScript, VCSMixin, BuildbotMixin):
>+                            all_actions=[
>+                                'setup-mock',
>+                                'checkout-servo',
>+                                'clobber-obj',
>+                                'configure',
>+                                'build',
>+                                'check',
>+                            ],
>+                            config={
>+                                'default_vcs': 'gittool',
>+                                'backup_rust': True,

You could also set this via optparse, e.g.

    [["--no-backup-rust"], {
        "dest": "backup_rust",
        "action": "store_false",
        "default": True,
        "help": "don't backup rust before clobbering",
    }],

Either way is fine.

>+    def checkout_servo(self):
>+        dirs = self.query_abs_dirs()
>+
>+        rev = self.vcs_checkout(
>+            repo=self.config.get('repo'),
>+            dest=dirs['abs_work_dir'],
>+            revision=self.config.get('revision'),
>+            branch=self.config.get('branch'),
>+        )
>+        self.set_buildbot_property('got_revision', rev, write_to_file=True)
>+
>+    def clobber_obj(self):
>+        dirs = self.query_abs_dirs()
>+
>+        if self.config.get('backup_rust') and os.path.exists(dirs['objdir']):
>+            self.run_command(['make', 'backup-rust'], cwd=dirs['objdir'],
>+                             halt_on_failure=True)
>+        self.rmtree(dirs['objdir'])

Hm.
Do we never want to clobber the entire tree?
That will save us time if true, but I'm at least somewhat skeptical.
PurgeMixin.clobber() may be useful if we do, or at least PurgeMixin.purge_builds() if we are at all worried about disk space issues in the future.

Also, we never want to run dep builds?
I think doing away with dep builds is a great thing if we can afford the time hit.

>+    def build(self):
>+        dirs = self.query_abs_dirs()
>+
>+        # If rust was backed up, we need to restore it before building,
>+        # otherwise it will get rebuilt from scratch.
>+        if self.config.get('backup_rust'):
>+            self.run_command(['make', 'restore-rust'], cwd=dirs['objdir'])

Aha, I think this answers my dep build question.

>+            self.fatal("Build failed, can't continue.", exit_code=FAILURE)

And this is a clever shorthand to get around using buildbot_status().


I don't think any of my comments are blockers to checking this in, though populating the MPL2 would be good.

Comment 1

[[e-mail] (:bhearsum)]

Not really blocking the Servo project, still going to get to this soon though.

Comment 2

[[e-mail] (:bhearsum)]

Just reading over these comments again...

(In reply to Ben Hearsum [:bhearsum] from comment #0)
> Since you do have a separate config file, it would be easy enough to have
> the script's default_actions be everything but setup-mock, and then override
> those default_actions in this sonfig file (with everything including
> setup-mock).
> However, making setup-mock noop is also acceptable, as I said in IRC.

While it's not ideal for command line usage, I've gone with add_actions in the config here, to avoid duplicating default_actions to it. I want to avoid future problems when we add new steps.

> >+SUCCESS, WARNINGS, FAILURE, EXCEPTION = xrange(4)
> 
> This is fine and may be superior to the current levels in
> mozharness.mozilla.buildbot, but it might be nice to standardize.

> >+            self.fatal("Build failed, can't continue.", exit_code=FAILURE)
> 
> And this is a clever shorthand to get around using buildbot_status().

So, the reason I avoided the things in mozharness.mozilla.buildbot is because they talk about TBPL, which Servo doesn't care about. If they're truly just reflecting things from Buildbot, can I rename them to something more generic? It'd be confusing to people to have Servo build scripts talking about TBPL, I think.

> >+class ServoBuild(MockMixin, BaseScript, VCSMixin, BuildbotMixin):
> >+                            all_actions=[
> >+                                'setup-mock',
> >+                                'checkout-servo',
> >+                                'clobber-obj',
> >+                                'configure',
> >+                                'build',
> >+                                'check',
> >+                            ],
> >+                            config={
> >+                                'default_vcs': 'gittool',
> >+                                'backup_rust': True,
> 
> You could also set this via optparse, e.g.
> 
>     [["--no-backup-rust"], {
>         "dest": "backup_rust",
>         "action": "store_false",
>         "default": True,
>         "help": "don't backup rust before clobbering",
>     }],
>
> Either way is fine.

Looks like I already did this.
 
> >+    def checkout_servo(self):
> >+        dirs = self.query_abs_dirs()
> >+
> >+        rev = self.vcs_checkout(
> >+            repo=self.config.get('repo'),
> >+            dest=dirs['abs_work_dir'],
> >+            revision=self.config.get('revision'),
> >+            branch=self.config.get('branch'),
> >+        )
> >+        self.set_buildbot_property('got_revision', rev, write_to_file=True)
> >+
> >+    def clobber_obj(self):
> >+        dirs = self.query_abs_dirs()
> >+
> >+        if self.config.get('backup_rust') and os.path.exists(dirs['objdir']):
> >+            self.run_command(['make', 'backup-rust'], cwd=dirs['objdir'],
> >+                             halt_on_failure=True)
> >+        self.rmtree(dirs['objdir'])
> 
> Hm.
> Do we never want to clobber the entire tree?
> That will save us time if true, but I'm at least somewhat skeptical.
> PurgeMixin.clobber() may be useful if we do, or at least
> PurgeMixin.purge_builds() if we are at all worried about disk space issues
> in the future.
> 
> Also, we never want to run dep builds?
> I think doing away with dep builds is a great thing if we can afford the
> time hit.
> 
> >+    def build(self):
> >+        dirs = self.query_abs_dirs()
> >+
> >+        # If rust was backed up, we need to restore it before building,
> >+        # otherwise it will get rebuilt from scratch.
> >+        if self.config.get('backup_rust'):
> >+            self.run_command(['make', 'restore-rust'], cwd=dirs['objdir'])
> 
> Aha, I think this answers my dep build question.

I don't think there's anything to do re: clobbers/dep builds. Current state of affairs is that full incremental builds are unsupported, and the backup/restore rust works around doing full megaclobbers every time. We may need full incremental build support in the future.

needsinfo? for the question about Buildbot/TBPL statuses.

Comment 3

[[e-mail] (:bhearsum)]

Oops, forgot to set the flag.

Comment 4

[Aki Sasaki (not active)]

(In reply to Ben Hearsum [:bhearsum] from comment #2)
> So, the reason I avoided the things in mozharness.mozilla.buildbot is
> because they talk about TBPL, which Servo doesn't care about. If they're
> truly just reflecting things from Buildbot, can I rename them to something
> more generic? It'd be confusing to people to have Servo build scripts
> talking about TBPL, I think.

I didn't think it was buildbot status, and TBPL status was more clear.
Another thing I like about the TBPL status strings is that they're unlikely to show up in any build/test logs (fairly unique).

However, I'm open to a rename if we can keep that fairly-unique quality.

Comment 5

[[e-mail] (:bhearsum)]

Attachment: address review comments

So, this addresses the setup-mock + duplication of exit codes. I renamed TBPL stuff to "script", because after reading over the usage of it in BuildbotMixin, it looks to me like it's just matching a convention we've set in our buildbot configs - not anything specific to Buildbot or TBPL. Unless I've misunderstood, it seem like the part of BuildbotMixin.buildbot_status that decides on an exit code belongs outside of BuildbotMixin. I happen to be the first person to be doing work on something that's not TBPL integrated, so I guess that's why I'm hitting this :).

I know this is invasive, so I'd appreciate some feedback on the approach before I do any testing (it does pass unit tests though).

Comment 6

[Aki Sasaki (not active)]

Comment on attachment 757421 [details] [diff] [review]
address review comments

(In reply to Ben Hearsum [:bhearsum] from comment #5)
> Created attachment 757421 [details] [diff] [review]
> address review comments
> 
> So, this addresses the setup-mock + duplication of exit codes.

I almost prefer the no-op setup-mock solution since it doesn't get wonky if someone uses the --add-action command line option.  I won't block on it, though.

> I renamed
> TBPL stuff to "script", because after reading over the usage of it in
> BuildbotMixin, it looks to me like it's just matching a convention we've set
> in our buildbot configs - not anything specific to Buildbot or TBPL.

Hm.
I'm not entirely sold on  the name SCRIPT_* .  The script can be run just fine without any of this; it's the external thing parsing it (buildbot and tbpl until servo came along) that requires these things set to know how to mark the status.  That's why TBPL_STATUS made sense to me.  I avoided the name BUILDBOT_STATUS because I was hoping/assuming buildbot would go away, and TBPL would stay.  Servo throws a monkey wrench into that assumption.  But it's still not the script that needs this; it's tbpl, buildbot, or whatever servo's using.

In a way I'm leaning towards keeping the TBPL_* names and just disguising the TBPL string from the log, unless servo or similar edge cases are going to become a significant percentage of our automation.

> Unless
> I've misunderstood, it seem like the part of BuildbotMixin.buildbot_status
> that decides on an exit code belongs outside of BuildbotMixin. I happen to
> be the first person to be doing work on something that's not TBPL
> integrated, so I guess that's why I'm hitting this :).

BuildbotMixin kind of handles anything involved with automating this stuff in buildbot, including downstream stuff.  I wanted a piece we replace completely when we move away from buildbot and onto LWR or whatever.  I'm not sure where else it would belong.

> I know this is invasive, so I'd appreciate some feedback on the approach
> before I do any testing (it does pass unit tests though).

Comment 7

[[e-mail] (:bhearsum)]

(In reply to Aki Sasaki [:aki] from comment #6)
> Comment on attachment 757421 [details] [diff] [review]
> address review comments
> 
> (In reply to Ben Hearsum [:bhearsum] from comment #5)
> > I renamed
> > TBPL stuff to "script", because after reading over the usage of it in
> > BuildbotMixin, it looks to me like it's just matching a convention we've set
> > in our buildbot configs - not anything specific to Buildbot or TBPL.
> 
> Hm.
> I'm not entirely sold on  the name SCRIPT_* .  The script can be run just
> fine without any of this; it's the external thing parsing it (buildbot and
> tbpl until servo came along) that requires these things set to know how to
> mark the status.  That's why TBPL_STATUS made sense to me.  I avoided the
> name BUILDBOT_STATUS because I was hoping/assuming buildbot would go away,
> and TBPL would stay.

So, I totally understand not going with Buildbot. I guess what's confusing to me is why it's in BuildbotMixin in that case. It feels like it should be elsewhere.

In any case, I'm going to shy away from touching this, as it's a pretty invasive change that we can't agree on! Is it OK if I just land the mock part of this?

Comment 8

[Aki Sasaki (not active)]

(In reply to Ben Hearsum [:bhearsum] from comment #7)
> So, I totally understand not going with Buildbot. I guess what's confusing
> to me is why it's in BuildbotMixin in that case. It feels like it should be
> elsewhere.

Mozharness only interfaces with TBPL through buildbot.
Also, I don't want to create even more mixins (a number of the ones we have could+should be consolidated until there's a need to split them).

> In any case, I'm going to shy away from touching this, as it's a pretty
> invasive change that we can't agree on! Is it OK if I just land the mock
> part of this?

Sure.

Comment 9

[[e-mail] (:bhearsum)]

Attachment: as landed