746546 - add more info to writemozinfo

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

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

in automation.py.in, we have a series of variables that are defined during build time substition via the .in filename extension.  We need to find a way to incorporate this into mozinfo.

The best method I can think of is creating a buildinfo.json.in which would result in buildinfo.json.  We can then seed this buildinfo.json into mozinfo.py and provide it through that api.

Here is a list of the expanded variables in automation.py.in:
#expand _DIST_BIN = __XPC_BIN_PATH__
#expand _IS_WIN32 = len("__WIN32__") != 0
#expand _IS_MAC = __IS_MAC__ != 0
#expand _IS_LINUX = __IS_LINUX__ != 0
#ifdef IS_CYGWIN
#expand _IS_CYGWIN = __IS_CYGWIN__ == 1
#else
_IS_CYGWIN = False
#endif
#expand _IS_CAMINO = __IS_CAMINO__ != 0
#expand _BIN_SUFFIX = __BIN_SUFFIX__
#expand _PERL = __PERL__

#expand _DEFAULT_APP = "./" + __BROWSER_PATH__
#expand _CERTS_SRC_DIR = __CERTS_SRC_DIR__
#expand _IS_TEST_BUILD = __IS_TEST_BUILD__
#expand _IS_DEBUG_BUILD = __IS_DEBUG_BUILD__
#expand _CRASHREPORTER = __CRASHREPORTER__ == 1


I believe we will need to provide:
DIST_BIN
DEFAULT_APP
CERTS_SRC_DIR
IS_TEST_BUILD
IS_DEBUG_BUILD
CRASHREPORTER

Comment 1

[(not currently active) Ted Mielczarek]

The simple solution here is just to expand writemozinfo, which we're already using for xpcshell tests:
http://mxr.mozilla.org/mozilla-central/source/config/writemozinfo.py

We already have debug and crashreporter keys in there, adding a key for --enable-tests would be easy. The paths are a little weird, but it should be straightforward to port the logic we're using from automation-build.mk and binary-location.mk:
http://mxr.mozilla.org/mozilla-central/source/build/automation-build.mk
http://mxr.mozilla.org/mozilla-central/source/build/binary-location.mk

Comment 2

[(not currently active) Ted Mielczarek]

Attachment: add more info to writemozinfo

This adds a whole bunch of stuff to writemozinfo:
tests_enabled (bool)
bin_suffix (string)
dist (path)
certs_srcdir (path)
appname (string, like 'firefox')
default_app (string, like 'firefox.exe' or 'Nightly.app/Contents/MacOS/firefox')

Comment 3

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

Comment on attachment 616341 [details] [diff] [review]
add more info to writemozinfo

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

thanks for the tests as well.

::: build/pgo/profileserver.py
@@ +83,5 @@
>    browserEnv["XPCOM_DEBUG_BREAK"] = "warn"
>    browserEnv["MOZ_JAR_LOG_DIR"] = MOZ_JAR_LOG_DIR
>  
>    url = "http://localhost:%d/index.html" % PORT
> +  appPath = "/Applications/Nightly.app/Contents/MacOS/firefox" #os.path.join(SCRIPT_DIR, automation.DEFAULT_APP)

I assume this was accidentally added in

::: config/tests/unit-writemozinfo.py
@@ +159,5 @@
>                          'MOZ_WIDGET_TOOLKIT':'gtk2',
>                          'MOZ_CRASHREPORTER':'1'})
>          self.assertEqual(True, d['crashreporter'])
>  
> +    def testTestsEnabled(self):

the name here made me chuckle.

Comment 4

[(not currently active) Ted Mielczarek]

Oops, good catch. I hacked that in to test something else.

Comment 5

[(not currently active) Ted Mielczarek]

I pushed this to try because it fixes bug 733501, so there are some xpcshell tests that will start being run that weren't previously being run. I'd like to sanity check that they're not all broken.

Comment 6

[(not currently active) Ted Mielczarek]

This patch failed on mac universal builds, because the $(DIST) paths are different for the two halves of the build, and we unify mozinfo.json into the test package. Yuck.

Comment 7

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

ted, are you looking into this?  do you want me or somebody else to look into it?

Comment 8

[(not currently active) Ted Mielczarek]

I think I just need to remove the path info from this patch, and sort it out in a different way in the harnesses.

Comment 9

[Andrew Halberstadt [:ahal]]

Ted, is this still something you are working on? I can take it over if you are swamped

Comment 10

[(not currently active) Ted Mielczarek]

Nope, go for it.

Comment 11

[Andrew Halberstadt [:ahal]]

Going to look into the refactor mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=866094#c7 at the same time

Comment 12

[Andrew Halberstadt [:ahal]]

Attachment: Patch 1.0 - use buildconfig module

Not sure if this is what you had in mind.. but this makes the most sense to me. Basically we create the same shortcuts as we did before, except the source info is coming from buildconfig instead of os.environ.

All the buildconfig information is copied over 1:1 to mozinfo.json (so you can still access it via mozinfo['substs'] or mozinfo['defines'] etc). This is probably overkill, but I guess it doesn't hurt.

There are few things that aren't defined in buildconfig like XPC_BIN_PATH (or DIST_BIN) and CERTS_SRC_DIR. For these I just manually built the paths based on their definitions in the Makefiles. If you know how to properly extract these values from Makefiles, please let me know

Comment 13

[Andrew Halberstadt [:ahal]]

Just noticed an instance of 'env' that I forgot to change to 'substs'. Fixed in my local patch.

Comment 14

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

Comment on attachment 748223 [details] [diff] [review]
Patch 1.0 - use buildconfig module

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

this seems pretty sane.

Comment 15

[(not currently active) Ted Mielczarek]

Comment on attachment 748223 [details] [diff] [review]
Patch 1.0 - use buildconfig module

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

::: config/writemozinfo.py
@@ +26,2 @@
>      d = {}
> +    d.update([(key, getattr(buildconfig, key)) for key in buildconfig.__all__])

I don't want to do this, there's a *lot* of stuff in buildconfig. I'd rather have a whitelist so we don't proliferate a bunch of useless stuff into mozinfo. Plus we explicitly sanitize some of the values into canonical forms below, I don't want people using the silly build-system-provided values instead of our nice clean values.

@@ +87,5 @@
> +        d['dist'] = os.path.join(d['topobjdir'], 'dist')
> +    d['dist_bin'] = os.path.join(d['dist'], 'bin')
> +
> +    if 'certs_srcdir' not in d:
> +        d['certs_srcdir'] = os.path.join(d['topsrcdir'], 'build', 'pgo', 'certs')

I thought I mentioned on the bug or on IRC, but I don't want to put paths in here. That's going to break Mac universal builds, and it's awkward. We should fix the hardcoded paths in a different way.

Comment 16

[Andrew Halberstadt [:ahal]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> ::: config/writemozinfo.py
> @@ +26,2 @@
> >      d = {}
> > +    d.update([(key, getattr(buildconfig, key)) for key in buildconfig.__all__])
> 
> I don't want to do this, there's a *lot* of stuff in buildconfig. I'd rather
> have a whitelist so we don't proliferate a bunch of useless stuff into
> mozinfo. Plus we explicitly sanitize some of the values into canonical forms
> below, I don't want people using the silly build-system-provided values
> instead of our nice clean values.

Sure, makes sense.

> @@ +87,5 @@
> > +        d['dist'] = os.path.join(d['topobjdir'], 'dist')
> > +    d['dist_bin'] = os.path.join(d['dist'], 'bin')
> > +
> > +    if 'certs_srcdir' not in d:
> > +        d['certs_srcdir'] = os.path.join(d['topsrcdir'], 'build', 'pgo', 'certs')
> 
> I thought I mentioned on the bug or on IRC, but I don't want to put paths in
> here. That's going to break Mac universal builds, and it's awkward. We
> should fix the hardcoded paths in a different way.

Afaict building these paths manually here isn't any different than how automation.py.in currently gets its DIST_BIN and CERTS_SRC_DIR values:
http://mxr.mozilla.org/mozilla-central/source/build/automation-build.mk#12
http://mxr.mozilla.org/mozilla-central/source/build/automation-build.mk#16

Like I mentioned, if you know of a better way to get these values let me know. But I'm not sure what else I can do.

Comment 17

[Andrew Halberstadt [:ahal]]

Attachment: Patch 2.0 - address review comments

On irc I learned that mac universal builds are even trickier than I thought. Unfortunately the reason I picked this bug up in the first place was primarily for DIST_BIN. Oh well, might as well finish this up while it's fresh in my mind.

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 749463 [details] [diff] [review]
Patch 2.0 - address review comments

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

Thanks, this looks like a nice cleanup. You should be able to remove all the junk around the writemozinfo invocation in configure now as well:
http://mxr.mozilla.org/mozilla-central/source/configure.in#9197

::: build/buildconfig.py
@@ +25,5 @@
>      value = getattr(config, var)
>      if isinstance(value, list) and value and isinstance(value[0], tuple):
>          value = dict(value)
>      setattr(sys.modules[__name__], var, value)
> +    __all__.append(var)

Doesn't look like you actually need these changes now, do you?

::: config/writemozinfo.py
@@ +32,5 @@
>                          ', '.join(missing))
>  
> +    d = {}
> +    d['topsrcdir'] = buildconfig.topsrcdir
> +    d['topobjdir'] = buildconfig.topobjdir

I think we should leave these out.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 749463 [details] [diff] [review]
Patch 2.0 - address review comments

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

I realized this is going to break the unit tests:
http://mxr.mozilla.org/mozilla-central/source/config/tests/unit-writemozinfo.py

You can probably fix this fairly simply by doing like
substs = env if env else buildconfig.substs

::: config/writemozinfo.py
@@ +32,5 @@
>                          ', '.join(missing))
>  
> +    d = {}
> +    d['topsrcdir'] = buildconfig.topsrcdir
> +    d['topobjdir'] = buildconfig.topobjdir

I didn't realize we already had topsrcdir in there. Yuck. In any event, I think putting objdir in there is going to break universal builds.

Comment 20

[Andrew Halberstadt [:ahal]]

Attachment: Patch 3.0 - address review comments

Regarding the __all__, it is technically correct to leave it in, but I removed it for the sake of the MNC (minimum necessary change).

Pushed some builds to try:
https://tbpl.mozilla.org/?tree=Try&rev=5533e97c4c33

Comment 21

[(not currently active) Ted Mielczarek]

Comment on attachment 751146 [details] [diff] [review]
Patch 3.0 - address review comments

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

Fix spelling in your commit message. :)

::: config/writemozinfo.py
@@ +22,5 @@
>      Build a dict containing data about the build configuration from
>      the environment.
>      """
> +    substs = env or buildconfig.substs
> +    env = env or os.environ

I think you can drop this line, the tests will pass the env dict in directly and otherwise we'll use buildconfig.substs.

Comment 22

[Andrew Halberstadt [:ahal]]

No, we still need os.environ for 'MOZCONFIG' which isn't getting set anywhere in buildconfig (at least for my configuration). Though the unit test is still failing on try. I'll fix it up and carry the r+ forward.

Comment 23

[Andrew Halberstadt [:ahal]]

Attachment: Patch 4.0 - fix unit test failures

Fix two failures in the unit test and carry the r+ forward.

Try run looks good: https://tbpl.mozilla.org/?tree=Try&rev=81ee28966c14

Comment 24

[Andrew Halberstadt [:ahal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f37f1883d489

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f37f1883d489