Port profileserver.py to Mozbase

RESOLVED FIXED in mozilla24

Status

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: ted, Assigned: ted)

Tracking

unspecified
mozilla24
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozbase], )

Attachments

(2 attachments, 1 obsolete attachment)

profileserver.py is pretty simple, but it uses automation.py. We should port it to use mozbase instead.
Assignee: nobody → jmaher
Depends on: 661908
Whiteboard: [mozbase]
bug 746546 should add enough info to mozinfo to replace the usage of automation.py. We'll probably want to copy mozinfo.json from the root of the objdir and stick it in $objdir/_profile/pgo, then just look for it next to profileserver.py like we do with runxpcshelltests.py.
Depends on: 746546
Blocks: 775756
bug 855262 adds the ability to get the default app location, which should be enough to fix this.
Depends on: 855262
Here's a first cut at this. It works locally, but there are at least two more things I need to fix to cover cases we use in automation. First, it needs to run the browser from the package staging location (dist/firefox), since that's what we do for PGO. Second, it needs to handle --debugger/--debugger-args, since we use that for Valgrind builds.
Assignee: jmaher → ted
Depends on: 875576
genpgocert.py is collateral damage while fixing profileserver.py, but I wound up almost rewriting it, so I split the first batch of cleanup into a separate patch.
Depends on: 878043
So this seems to work locally, I did a PGO build on Linux and it looks alright. I'll push it to try and make sure that Windows and Linux PGO builds are okay there.

There's a lot of churn here, because not only did I basically rewrite profileserver.py and genpgocert.py, but I also made them non-preprocessed while I was at it, and got rid of copying everything to the objdir, since that's no longer necessary, so I was able to completely remove the Makefiles under build/pgo while I was at it.

At the end of the day the code looks pretty sensible though!
Attachment #756678 - Flags: review?(jhammel)
Attachment #752991 - Attachment is obsolete: true
Attachment #756146 - Flags: review?(jmaher)
Comment on attachment 756146 [details] [diff] [review]
clean up a few things in genpgocert.py

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

::: build/pgo/genpgocert.py.in
@@ +70,5 @@
> +    unlinkDbFiles(tempDbDir)
> +
> +    # Create temporary certification database for CA generation
> +    status = runUtil(certutil, ["-N", "-d", tempDbDir, "-f", pwfile.name])
> +    if status != 0:

Does `if status` work?
Comment on attachment 756678 [details] [diff] [review]
Port profileserver.py to Mozbase

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

::: build/pgo/blueprint/moz.build
@@ -1,2 @@
> -# -*- Mode: python; c-basic-offset: 4; indent-tabs-mode: nil; tab-width: 40 -*-
> -# vim: set filetype=python:

Do we require/desire these modelines? just curious

::: build/pgo/genpgocert.py
@@ +7,3 @@
>  from mozfile import NamedTemporaryFile
> +import mozinfo
> +from mozprofile.permissions import ServerLocations

Nit: I'm in favor of having the `from` imports and the `import` imports together when applicable (may/may not be part of some PEP). It'd be nice if these lines were reversed, but also not that important.

@@ +40,5 @@
>  def runUtil(util, args, inputdata = None):
> +  env = os.environ.copy()
> +  if mozinfo.os == "linux":
> +    pathvar = "LD_LIBRARY_PATH"
> +    app_path = os.path.dirname(util)

Any issues with absolute paths v relative paths here?

@@ +48,5 @@
> +      env[pathvar] = app_path
> +  proc = subprocess.Popen([util] + args, env=env,
> +                          stdin=subprocess.PIPE if inputdata else None)
> +  proc.communicate(inputdata)
> +  return proc.returncode

Is there any value in upstreaming this code to....somewhere? (e.g. mach/mozbuid/lordknows)

@@ +116,5 @@
>      # Generate automatic certificate
> +    locations = ServerLocations(os.path.join(build.topsrcdir,
> +                                             "build", "pgo",
> +                                             "server-locations.txt"))
> +    iterator = iter(locations)

Maybe I'm forgetting something....but why do we have to do iter() here? (It should work fine just doing the for loc in locations, I think)

@@ +118,5 @@
> +                                             "build", "pgo",
> +                                             "server-locations.txt"))
> +    iterator = iter(locations)
> +    # Skips the first entry, I don't know why.
> +    iterator.next()

I'd love a follow-up to figure this out, even though I don't know who/when will have time to solve a random mystery.

I'd also love an extra line of space so the context is incredibly obvious [http://www.imdb.com/title/tt0057012/quotes?item=qt0454454]

@@ +157,3 @@
>  if len(sys.argv) == 1:
>    print "Specify --gen-server or --gen-ca"
>    sys.exit(1)

Of course I'd just love a "proper" script with `if __name__ == '__main__':` and maybe optparse/argparse, but out of scope here and not important until/if its needed
Attachment #756678 - Flags: review?(jhammel) → review+
Thanks for the review. The genpgoserver script could use a lot more love, I cleaned up the stuff I thought was important/obvious and the stuff that was necessary to change from automation.py to mozbase, but it's still a little clunky. I also didn't think it was quite as important given that it's just a utility script, not an actual harness (it just regenerates the certs used for SSL mochitests).
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8)
> Thanks for the review. The genpgoserver script could use a lot more love, I
> cleaned up the stuff I thought was important/obvious and the stuff that was
> necessary to change from automation.py to mozbase, but it's still a little
> clunky. I also didn't think it was quite as important given that it's just a
> utility script, not an actual harness (it just regenerates the certs used
> for SSL mochitests).

Sure, hence the r+ :) Most of the issues I noted were nits and suggestions (and curiosity questions); its ABICT fine for checkin as is, though if you want to make minor changes suggested, that's cool too. (And, of course, as you point out its not like the script was pristine before your patch; its undoubtedly an improvement.)
The Try run looked good, although I apparently only forced PGO on for the Windows build, not the Linux builds:
https://tbpl.mozilla.org/?tree=Try&rev=bf4e120cf318

I looked at the Windows log and verified that it ran the script and that profiling info was used in the final link.
Comment on attachment 756146 [details] [diff] [review]
clean up a few things in genpgocert.py

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

just a few nits, this looks more like minor cleanups.

::: build/pgo/genpgocert.py.in
@@ +83,5 @@
> +    if status != 0:
> +      return status
> +
> +    status = runUtil(pk12util, ["-o", pgoCAModulePathSrc, "-n", "pgo temporary ca", "-d", tempDbDir, "-w", pwfile.name, "-k", pwfile.name])
> +    if status != 0:

more checks that could be 'if status'

@@ +136,5 @@
> +
> +          if firstLocation == "":
> +            firstLocation = loc.host
> +
> +    if firstLocation == "":

if not firstLocation?
Attachment #756146 - Flags: review?(jmaher) → review+
(In reply to Jeff Hammel [:jhammel] from comment #7)
> Do we require/desire these modelines? just curious

Not required, but desired, yes, since otherwise editors don't enable Python mode for moz.build files.

> Nit: I'm in favor of having the `from` imports and the `import` imports
> together when applicable (may/may not be part of some PEP). It'd be nice if
> these lines were reversed, but also not that important.

I didn't know if we had a preferred style here, I'll rearrange them.

> Any issues with absolute paths v relative paths here?

MozbuildObject returns all absolute paths now, so this shouldn't be a problem.

> Is there any value in upstreaming this code to....somewhere? (e.g.
> mach/mozbuid/lordknows)

Which part, the LD_LIBRARY_PATH fiddling? I would like to have that somewhere, I think we've talked about this before, probably in the context of:
http://mxr.mozilla.org/mozilla-central/source/testing/runcppunittests.py#77

> Maybe I'm forgetting something....but why do we have to do iter() here? (It
> should work fine just doing the for loc in locations, I think)

Because it wants to pop off the first item. I'm not sure why, but I don't really want to screw with it.

> > +    # Skips the first entry, I don't know why.
> > +    iterator.next()
> 
> I'd love a follow-up to figure this out, even though I don't know who/when
> will have time to solve a random mystery.

I'll file it, mayhemer wrote this, perhaps he remembers.

> Of course I'd just love a "proper" script with `if __name__ == '__main__':`
> and maybe optparse/argparse, but out of scope here and not important
> until/if its needed

Yeah, I think as long as this script still works it's not incredibly important.

Thanks for the review!
(In reply to Joel Maher (:jmaher) from comment #11)
> Comment on attachment 756146 [details] [diff] [review]
> clean up a few things in genpgocert.py
> 
> Review of attachment 756146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> just a few nits, this looks more like minor cleanups.

Thanks for the review! If it's alright with you I'll fold the nit fixes you requested into the second patch to make my life easier.
(In reply to Jeff Hammel [:jhammel] from comment #7)
> > +    # Skips the first entry, I don't know why.
> > +    iterator.next()
> 
> I'd love a follow-up to figure this out, even though I don't know who/when
> will have time to solve a random mystery.


Filed bug 879740 on this.
> > Is there any value in upstreaming this code to....somewhere?
> (e.g.
> > mach/mozbuid/lordknows)
> Which part, the LD_LIBRARY_PATH fiddling? I would like to have that
> somewhere, I think we've talked about this before, probably in the
> context of:
> http://mxr.mozilla.org/mozilla-
> central/source/testing/runcppunittests.py#77

Yep, that thing. :) Sounds good

> > Maybe I'm forgetting something....but why do we have to do iter()
> here? (It
> > should work fine just doing the for loc in locations, I think)

> Because it wants to pop off the first item. I'm not sure why, but I
> don't really want to screw with it.

Not how I would do it, but fair 'nuff
I was rewriting enough of this file that I didn't want to risk breaking something while I was there. We'll see if we can get an answer in the followup bug. I'll also see if I ever filed a bug about moving that LD_LIBRARY_PATH code somewhere, and if not I'll file one.
https://hg.mozilla.org/mozilla-central/rev/5d8bf758f408
https://hg.mozilla.org/mozilla-central/rev/8b379faea096
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Depends on: 934542
You need to log in before you can comment on or make changes to this bug.