746244 - Port profileserver.py to Mozbase

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[(not currently active) Ted Mielczarek]

profileserver.py is pretty simple, but it uses automation.py. We should port it to use mozbase instead.

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[(not currently active) Ted Mielczarek]

bug 855262 adds the ability to get the default app location, which should be enough to fix this.

Comment 3

[(not currently active) Ted Mielczarek]

Attachment: Port profileserver.py to Mozbase (WIP)

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.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: clean up a few things in genpgocert.py

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.

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: Port profileserver.py to Mozbase

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!

Comment 6

[Jeff Hammel]

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 7

[Jeff Hammel]

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

Comment 8

[(not currently active) Ted Mielczarek]

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).

Comment 9

[Jeff Hammel]

(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.)

Comment 10

[(not currently active) Ted Mielczarek]

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 11

[Joel Maher ( :jmaher ) (UTC -8)]

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?

Comment 12

[(not currently active) Ted Mielczarek]

(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!

Comment 13

[(not currently active) Ted Mielczarek]

(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.

Comment 14

[(not currently active) Ted Mielczarek]

(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.

Comment 15

[(not currently active) Ted Mielczarek]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5d8bf758f408
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b379faea096

Comment 16

[Jeff Hammel]

> > 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

Comment 17

[(not currently active) Ted Mielczarek]

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.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5d8bf758f408
https://hg.mozilla.org/mozilla-central/rev/8b379faea096