Closed Bug 1313099 Opened 3 years ago Closed 3 years ago

Try pulling node from tooltool

Categories

(Core :: Networking, defect)

51 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: u408661, Assigned: u408661)

References

Details

(Whiteboard: [necko-active])

Attachments

(2 files, 2 obsolete files)

As a way to help speed the recovery from bug 1248198, :catlee suggested via email trying to put node into tooltool (so most changes would be in-tree, avoid issues like the above). This is to track my work on attempting that.
MozReview-Commit-ID: 4JvqlqlKLTC
MozReview-Commit-ID: 7ijnlF5D9B2
Comment on attachment 8807280 [details] [diff] [review]
Put nodejs in tooltool (m-c changes)

Hi Jordan - :catlee said you were one of the go-to people for mozharness, and when looking into this bug, I saw you were the one who added support for pulling minidump_stackwalk from tooltool. After cribbing a lot of the work in these two patches from your code for that, I decided to ask you for review. Hope that's ok (feel free to forward if not!)

This just contains the manifests for the node binaries/tarballs for the various platforms. The other patch on this bug contains the mozharness changes.
Attachment #8807280 - Flags: review?(jlund)
Comment on attachment 8807282 [details] [diff] [review]
Put node in tooltool (mozharness changes)

Here are the mozharness changes, as I said, quite a bit cribbed from your changes for minidump_stackwalk.

Beyond the question of "is this ok to land", I have no idea how to actually land this once I get r+ - mozharness has its own repo, but there's also an in-tree copy (which is what I used for my development, and what this patch is from). So any guidance there would be appreciated. Thanks!
Attachment #8807282 - Flags: review?(jlund)
Oh, and here's a try run with proof that at least my code does what it's supposed to do: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dcb31600a7be (even running http/2 tests on windows now!)
(In reply to Nicholas Hurley [:nwgh][:hurley] from comment #5)
> running http/2 tests on windows now!)

+1!
Comment on attachment 8807282 [details] [diff] [review]
Put node in tooltool (mozharness changes)

Review of attachment 8807282 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for writing this patch! It looks great overall with only one issue re: unpacking (see inline). Now that the list of manifests have grown >1, I think this would greatly benefit from some DRYness optimization. e.g. `for manifest in tooltool_manifests: # sanity check for manifest, paths; download items`. But that might be a bit beyond the scope of what you are trying to accomplish so I won't block on that.

sanity check: Looks like these node items have already been uploaded to the tooltool server? And nodejs is built into the image for Taskcluster jobs hence the `remove_executables` part?

I'm going to r- for now until you either comment below or after you have tried adding `"unpack": true` to the manifests that contain tarballs. It might need something additional in terms of where things are unpacked, etc, but it also might 'just work' :)

::: testing/mozharness/mozharness/mozilla/testing/testbase.py
@@ +723,5 @@
> +        abs_nodejs_path = os.path.join(dirs['abs_work_dir'], nodejs_path)
> +
> +        platform_name = self.platform_name()
> +        if platform_name not in ('win32', 'win64'):
> +            # Windows we download the binary directly. For everything else, it's in a tarball

iirc, https://github.com/mozilla/build-tooltool/blob/master/tooltool.py should provide some unpacking ability builtin. Have you tried passing: "unpack": true to the non windows nodejs manifests?

see examples here: https://dxr.mozilla.org/mozilla-central/search?q=path%3Atooltool%20unpack&=mozilla-central

::: testing/mozharness/scripts/desktop_unittest.py
@@ +695,5 @@
>  
>                  if self.query_minidump_stackwalk():
>                      env['MINIDUMP_STACKWALK'] = self.minidump_stackwalk_path
> +                if self.query_nodejs():
> +                    env['MOZ_NODE_PATH'] = self.nodejs_path

out of curiosity, where is MOZ_NODE_PATH digested? I tried grepping gecko and only saw it defined in taskcluster/xpcshell test definitions.
Attachment #8807282 - Flags: review?(jlund) → review-
Comment on attachment 8807280 [details] [diff] [review]
Put nodejs in tooltool (m-c changes)

Review of attachment 8807280 [details] [diff] [review]:
-----------------------------------------------------------------

see comments in other patch. resetting review for now. feel free to r? me again when you're ready.
Attachment #8807280 - Flags: review?(jlund)
(In reply to Jordan Lund (:jlund) from comment #7)
> sanity check: Looks like these node items have already been uploaded to the
> tooltool server? And nodejs is built into the image for Taskcluster jobs
> hence the `remove_executables` part?

Yes and yes.

> I'm going to r- for now until you either comment below or after you have
> tried adding `"unpack": true` to the manifests that contain tarballs. It
> might need something additional in terms of where things are unpacked, etc,
> but it also might 'just work' :)
> 
> ::: testing/mozharness/mozharness/mozilla/testing/testbase.py
> @@ +723,5 @@
> > +        abs_nodejs_path = os.path.join(dirs['abs_work_dir'], nodejs_path)
> > +
> > +        platform_name = self.platform_name()
> > +        if platform_name not in ('win32', 'win64'):
> > +            # Windows we download the binary directly. For everything else, it's in a tarball
> 
> iirc, https://github.com/mozilla/build-tooltool/blob/master/tooltool.py
> should provide some unpacking ability builtin. Have you tried passing:
> "unpack": true to the non windows nodejs manifests?
> 
> see examples here:
> https://dxr.mozilla.org/mozilla-central/
> search?q=path%3Atooltool%20unpack&=mozilla-central

I haven't, wasn't aware it existed! I'll give that a try and circle back around once I have the results of my attempt.

> ::: testing/mozharness/scripts/desktop_unittest.py
> @@ +695,5 @@
> >  
> >                  if self.query_minidump_stackwalk():
> >                      env['MINIDUMP_STACKWALK'] = self.minidump_stackwalk_path
> > +                if self.query_nodejs():
> > +                    env['MOZ_NODE_PATH'] = self.nodejs_path
> 
> out of curiosity, where is MOZ_NODE_PATH digested? I tried grepping gecko
> and only saw it defined in taskcluster/xpcshell test definitions.

This is used in testing/xpcshell/runxpcshelltests.py (specifically in trySetupNode, where we attempt to start the server we need for http/2 testing before running any xpcshell tests). Gecko itself doesn't care :)
Attachment #8807725 - Flags: review?(jlund)
Attachment #8807280 - Attachment is obsolete: true
Attachment #8807726 - Flags: review?(jlund)
Attachment #8807282 - Attachment is obsolete: true
As we can see from https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d8ab0815545 - using tooltool's "unpack" feature works wonderfully. Updated patches attached!
Comment on attachment 8807726 [details] [diff] [review]
Put node in tooltool (mozharness changes)

Review of attachment 8807726 [details] [diff] [review]:
-----------------------------------------------------------------

nice! thanks for trying that and giving context to other parts. lgmt :)

as to landing: ignore the isolated mozharness repo as, and mentioned more verbosely in the README, we use it for backporting 2 isolated releng projects: https://hg.mozilla.org/build/mozharness/file/3e9c78e32510/README.md

the true home of Mozharness lives in-tree so feel free to land on inbound (+ whatever gecko repos you need this for) like any other mozilla-central patch :D

thanks again for self-serving this.
Attachment #8807726 - Flags: review?(jlund) → review+
Attachment #8807725 - Flags: review?(jlund) → review+
Pushed by hurley@todesschaf.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d05d3e09e51
Put nodejs in tooltool (m-c changes) r=jlund
https://hg.mozilla.org/integration/mozilla-inbound/rev/231272fa95c5
Put nodejs in tooltool (mozharness changes) r=jlund
Thanks for the speedy re-review! Inbound should be good, we can let this ride the trains :)
(In reply to Pulsebot from comment #16)
> Pushed by philringnalda@gmail.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3940d0725fad
> followup, add missing comma

Wow, not sure how that happened (I did run it through try, maybe I fat-figered something when rebasing?) Anyway, thanks!
Duplicate of this bug: 1055707
Duplicate of this bug: 1310853
Duplicate of this bug: 1283871
You need to log in before you can comment on or make changes to this bug.