677207 - makefiles: standalone python script needed to generate values for a platform specific makefile

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

1) Create a standalone script w/o dependencies able to generate key/values pairs for platform specific values (uname, platform, cpu#, os_address_size, ...) values used in the sandbox (Darwin_Version, ?).  Also boolean values to simplify conditionals and ifdef/case blocks (isBSD, isLINUX, isMAC, isWINDOWS, isMINGW, isCYGWIN, isMOBILE, etc).

2) This script might be used before configure is launched so it should have no dependencies on external or generated content { aside from python }.

3) Output from the script can be used to generate a static include-able makefile.  Values should help reduce the need for sandbox makefiles having to launch shell commands to determine values.

4) Optional, a command line argument could be passed to alter the output format so the script could be used in different contexts or called from the shell to define variables.

5) Script can be invoked by the configure:: target in client.mk.  Dependencies should be used on only invoke the script when the generated makefile does not exist or the generation script has changed.

6) As a rider also add a makefile library target to generate directories in a thread safe manner (ie .done/ directories).  The rule can be used in several sandbox makefiles.  Optimal answer might be to define a new make macro that dependencies can be assigned to that would abstract derived names but that might be an activity for round 2.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

What do you need this for that configure can't handle?

Comment 2

[Joey Armstrong [:joey]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> What do you need this for that configure can't handle?

A dependency on configure would introduce a chicken-n-egg problem.  Container makefiles will need access to platform values for making conditional decisions and those would not be available until after configure has run.  Also only a small subset of variables are needed, configure is a large hammer.

Hmmm, long term it might simplify maintenance to shift some elements like this out of configure and into a hierarchy standalone python scripts [ not planned just speculating ].
  o Setup a simple script + unit test so they can be maintained as a set based on functionality.
  o configure remains small but could invoke scripts directly when values are needed.
    configure's involvement becomes verify python is at ver 2.5+ then delegate.
  o python syntax should open up configure maintenance to a wider audience

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I don't understand why anything needs to run before configure.  The basic premise of our build system is that you use client.mk to run configure which generates all the other makefiles needed.  What's are we gaining by changing that?

Rewriting configure in python has been discussed before.  Ted?

Comment 4

[(not currently active) Ted Mielczarek]

Yeah. I think we should just do this as part of configure for now. I chatted with Joey about our configure-in-python plan, sounds like it matches up with what he's planning fairly well.

Comment 5

[Joey Armstrong [:joey]]

Attachment: script and makefile used to generate a platform makefile

First attempt at a small python config script: config/makefiles/platformKV.py
Generate a key=value pair stream of collected platform values
Stream can be formatted with shell syntax[default] or makefile [--type make]
Tested on cygwin, linux, mac and mingw

client.mk - configure target overloaded to $(MAKE) -f makefilegen.mk after processing normal targets.

config/makefiles/makefilegen.mk - invoke platformKV.py with dependencies to generate an include-able platform makefile.

Sample platform output can be found here:
http://wiki.mozilla.org/User:JoeyArmstrong/makefiles/generated

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 552420 [details] [diff] [review]
script and makefile used to generate a platform makefile

Ted claims to understand what we're doing here, and I don't, so redirecting appropriately.

Comment 7

[Joey Armstrong [:joey]]

ping on the code review

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 552420 [details] [diff] [review]
script and makefile used to generate a platform makefile

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

r-, mostly because I think this ought to be run as part of configure. I'm also very picky about my Python, so there are a number of things I'd like to see cleaned up. I didn't get through all of this, but I figured I'd give you the feedback and I can look at the rest in another pass.

::: client.mk
@@ +302,5 @@
>  endif
>  
>  configure-files: $(CONFIGURES)
>  
> +configure::\

Nit: one space before the continuation backslash. (Here and elsewhere.)

@@ +340,5 @@
> +  makefilegen-platform\
> +  $(NULL)
> +
> +makefilegen-platform:
> +	$(MAKE) -f $(TOPSRCDIR)/config/makefiles/makefilegen.mk MOZ_OBJDIR=$(MOZ_OBJDIR) TOPSRCDIR=$(TOPSRCDIR)

I thought we discussed this and I said that this generation ought to happen as part of configure? I think it makes a lot more sense there, since that's where we generate our normal Makefiles.

::: config/makefiles/makefilegen.mk
@@ +1,4 @@
> +# -*- makefile -*-
> +###########################################################################
> +## Intent: Makefile used to launch container makefile generation with deps
> +###########################################################################

This whole Makefile kind of feels like overkill. You're just launching one command, you should just do that from configure or client.mk.

@@ +37,5 @@
> +PLATFORMGEN_ARGS = --type makefile
> +
> +platformgen: $(PLATFORMGEN_DONE)
> +platformgen.force:
> +	@touch -t 197001010101 $(PLATFORMGEN_DONE)

This is kind of a weird way to do this. Can't you just rm $(PLATFORMGEN_DONE)?

::: config/makefiles/platformKV.py
@@ +74,5 @@
> +##   cygwin - 2.6.5
> +##    mingw - 2.6.5
> +###########################################################################
> +def init():
> +    global pyver

I'm not a fan of globals. Please avoid them at all costs.

@@ +80,5 @@
> +        tmp = platform.python_version_tuple()      # platform.version="1.2.3+"
> +        pyver = str(tmp[0]) + '.' + str(tmp[1]) + '.0'
> +    except: # predates 2.3
> +        pyver = "2.3.0"
> +

This doesn't seem useful. We already check (and require) Python 2.5 or newer in configure.

@@ +85,5 @@
> +    if (version.StrictVersion('2.3.0') >= version.StrictVersion(pyver)):
> +        print 'WARNING: Your python version is old, upgrade to at least 2.5'
> +
> +    getUNAME() # caching read
> +    # /def init

These closing comments don't add much value. Python isn't C. :)

@@ +93,5 @@
> +##
> +## -----------------------------------------------------------------------
> +## TODO: Add more platform support - android, hp, solaris, etc
> +###########################################################################
> +def identify():

I gather you cribbed this from elsewhere? It looks like you have support for platforms we don't care about, like cygwin.

@@ +94,5 @@
> +## -----------------------------------------------------------------------
> +## TODO: Add more platform support - android, hp, solaris, etc
> +###########################################################################
> +def identify():
> +    uname = getUNAME()

So, I don't see how this is going to do the right thing, in general. You're detecting the build system type, but usually what's more important is the target system, which you can't detect this way. I have an existing Python script that does something similar, but uses configure variables to invoke itself:
http://mxr.mozilla.org/mozilla-central/source/configure.in#9379
http://mxr.mozilla.org/mozilla-central/source/config/writemozinfo.py

@@ +102,5 @@
> +
> +    if (-1 != arch.find('bsd')):
> +        rec = {
> +            'arch_common': 'bsd',
> +              'arch_type': uname[0].lower() # freebsd, netbsd, openbsd

These lines don't seem to line up. Here and elsewhere in this function.

@@ +153,5 @@
> +        elif (buffer[0] == '7'): archtype='windows7'
> +        elif platform.isVista(): archtype='vista'
> +#       buffer2 = sys.getwindowsversion() # 0-32 on win3.1, 1-95/98/me, 2-nt/2000/xp/x64, 3-ce
> +#       if getattr(sys, 'getwindowsversion', None) is not None:
> +#           sys.getwindowsversion()[0] == 6

If you're not going to use this code, just delete it. Leaving it in but commented out is confusing.

@@ +213,5 @@
> +        num = multiprocessing.cpu_count()
> +    except (ImportError, NotImplementedError):
> +        num = 0
> +
> +    if (0 == num): # Windows - also usable as an environment override

The parentheses here are superfluous, please leave them off. (Here and elsewhere.)

@@ +221,5 @@
> +                num = tmp
> +        except: # compile failure w/o this
> +            pass
> +
> +    if (0 == num): num = 1

Please don't put statements on the same line as an if statement. (Here and elsewhere.)

@@ +224,5 @@
> +
> +    if (0 == num): num = 1
> +
> +    # cpu + core
> +    showVarVal('hw_num_processors', str(num))

The str() here isn't really necessary, Python will stringify it for you when you pass it to the string formatter.

@@ +245,5 @@
> +        print "## " + arch + " legacy makefile shell macros"
> +        print "##"
> +        ver = platform.mac_ver()
> +        showVarVal('Darwin_Version', str(ver[0]))
> +    # /def legacyVars

I don't understand what this function's purpose is. Are you intending to flesh it out more in the future, and this is just a placeholder right now?

@@ +256,5 @@
> +uname_static = {}
> +def getUNAME():
> +    global uname_static
> +
> +    if (0 != len(uname_static)):

You can just write "if uname_static:" in cases like these. Empty lists/dicts/strings evaluate to False.

@@ +260,5 @@
> +    if (0 != len(uname_static)):
> +        return uname_static
> +
> +    try:
> +        uname = list(os.uname())

I've generally had better success with the platform module, FWIW:
http://docs.python.org/library/platform.html#module-platform

@@ +374,5 @@
> +
> +##----------------##
> +##---]  MAIN  [---##
> +##----------------##
> +def main(argv0):

This parameter seems...misnamed.

@@ +377,5 @@
> +##----------------##
> +def main(argv0):
> +
> +    try: # portability: argparse not available until 2.7
> +        opts, args = getopt.getopt(argv0,

Generally we use optparse: http://docs.python.org/library/optparse.html

but since you only have a few options getopt is probably fine. I don't understand the comment about argparse above, if you're not using argparse then why mention it?

Comment 9

[Joey Armstrong [:joey]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #8)
> Comment on attachment 552420 [details] [diff] [review]
> script and makefile used to generate a platform makefile
> 
> Review of attachment 552420 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r-, mostly because I think this ought to be run as part of configure. I'm
> also very picky about my Python, so there are a number of things I'd like to
> see cleaned up. I didn't get through all of this, but I figured I'd give you
> the feedback and I can look at the rest in another pass.
>
> > +makefilegen-platform:
> > +	$(MAKE) -f $(TOPSRCDIR)/config/makefiles/makefilegen.mk MOZ_OBJDIR=$(MOZ_OBJDIR) TOPSRCDIR=$(TOPSRCDIR)
> 
> I thought we discussed this and I said that this generation ought to happen
> as part of configure? I think it makes a lot more sense there, since that's
> where we generate our normal Makefiles.

The script is being launched as part of configure -- granted the call was not embedded and directly invoked by configure.sh but this is being called as a final dependency of the configure target.


> ::: config/makefiles/makefilegen.mk
> @@ +1,4 @@
> > +# -*- makefile -*-
> > +###########################################################################
> > +## Intent: Makefile used to launch container makefile generation with deps
> > +###########################################################################
> 
> This whole Makefile kind of feels like overkill. You're just launching one
> command, you should just do that from configure or client.mk.

One benefit added by the makefile are dependencies.  The script will only be launched as needed, when the generated file does not exist or platformKV.py is modified.  W/o deps the script must launched incessantly with each build.


> @@ +37,5 @@
> > +PLATFORMGEN_ARGS = --type makefile
> > +
> > +platformgen: $(PLATFORMGEN_DONE)
> > +platformgen.force:
> > +	@touch -t 197001010101 $(PLATFORMGEN_DONE)
> 
> This is kind of a weird way to do this. Can't you just rm
> $(PLATFORMGEN_DONE)?

I'll have to go back and try to reproduce the problem but if I remember correctly removing rather than touching the file produced a stray non existent file warning.


> @@ +94,5 @@
> > +## -----------------------------------------------------------------------
> > +## TODO: Add more platform support - android, hp, solaris, etc
> > +###########################################################################
> > +def identify():
> > +    uname = getUNAME()
> 
> So, I don't see how this is going to do the right thing, in general. You're
> detecting the build system type, but usually what's more important is the
> target system, which you can't detect this way. I have an existing Python
> script that does something similar, but uses configure variables to invoke
> itself:
> http://mxr.mozilla.org/mozilla-central/source/configure.in#9379
> http://mxr.mozilla.org/mozilla-central/source/config/writemozinfo.py

This was only the initial rev, tested on 4 available platforms.  It has not progressed far enough yet to support hosted android/maemo builds on linux/ex.  I had been thinking about simply passing an override value in as extra command line options and the platform values could be adjusted accordingly.

ps>  It would be very handy to be able to use scripts like these as a standalone entity w/o dependencies.  If configure is a prereq for being able to determine platform type, testing becomes a little more difficult in that static copies of configure output must be retained for the script to act on.

I'll have a look at the config logic.


> @@ +102,5 @@
> > +
> > +    if (-1 != arch.find('bsd')):
> > +        rec = {
> > +            'arch_common': 'bsd',
> > +              'arch_type': uname[0].lower() # freebsd, netbsd, openbsd
> 
> These lines don't seem to line up. Here and elsewhere in this function.

just a style difference, they were aligned on the colon separating key/value pairs rather than on the quote in the left hand column.
 

> @@ +153,5 @@
> > +        elif (buffer[0] == '7'): archtype='windows7'
> > +        elif platform.isVista(): archtype='vista'
> > +#       buffer2 = sys.getwindowsversion() # 0-32 on win3.1, 1-95/98/me, 2-nt/2000/xp/x64, 3-ce
> > +#       if getattr(sys, 'getwindowsversion', None) is not None:
> > +#           sys.getwindowsversion()[0] == 6
> 
> If you're not going to use this code, just delete it. Leaving it in but
> commented out is confusing.

The value is being used:
  o os_arch_common would be the generic class name 'windows'
  o os_arch_type set by the block above would be the flavor of windows - 7/2000/xp/nt/...

+    print
+    print "##"
+    print "## Platform Defines"
+    print "##"
+    showVarVal('os_arch_type',   rec['arch_type'])     # ubuntu
+    showVarVal('os_arch_common', rec['arch_common'])   # linux


> @@ +213,5 @@
> > +        num = multiprocessing.cpu_count()
> > +    except (ImportError, NotImplementedError):
> > +        num = 0
> > +
> > +    if (0 == num): # Windows - also usable as an environment override
> 
> The parentheses here are superfluous, please leave them off. (Here and
> elsewhere.)

Edits pending.  I'll start moving these suggestions over into the coding style guide to fill out a python section for posterity:
   http://developer.mozilla.org/En/Mozilla_Coding_Style_Guide


> @@ +245,5 @@
> > +        print "## " + arch + " legacy makefile shell macros"
> > +        print "##"
> > +        ver = platform.mac_ver()
> > +        showVarVal('Darwin_Version', str(ver[0]))
> > +    # /def legacyVars
> 
> I don't understand what this function's purpose is. Are you intending to
> flesh it out more in the future, and this is just a placeholder right now?

The intent was to be able to remove extra makefiles shell commands that could be replaced by static values.  One makefile in the sandbox was setup to call uname then valeus were copied into a "Darwin_Version" macro.  If the platform script or configure were to define the value extra shell overhead can be removed.


> @@ +260,5 @@
> > +    if (0 != len(uname_static)):
> > +        return uname_static
> > +
> > +    try:
> > +        uname = list(os.uname())
> 
> I've generally had better success with the platform module, FWIW:
> http://docs.python.org/library/platform.html#module-platform

I did try platform early on but the function was not quite as portable as uname.  MinGW was a problem child in it's own rigt, both platform and uname modules were unable to return a usable value.


> @@ +374,5 @@
> > +
> > +##----------------##
> > +##---]  MAIN  [---##
> > +##----------------##
> > +def main(argv0):
> 
> This parameter seems...misnamed.

Second rename request, sys.argv() was passed in to avoid referencing globals.  The parameter name could be name opts, args, argv0, whatever it just needs to be unique.  argv0 seemed like a mnemonic for where it came from.


> @@ +377,5 @@
> > +##----------------##
> > +def main(argv0):
> > +
> > +    try: # portability: argparse not available until 2.7
> > +        opts, args = getopt.getopt(argv0,
> 
> Generally we use optparse: http://docs.python.org/library/optparse.html
> 
> but since you only have a few options getopt is probably fine. I don't
> understand the comment about argparse above, if you're not using argparse
> then why mention it?

It can probably be removed, Justin sent me some info about optparse earlier.  There is a message in the module docs that as of python v2.7 the optparse module has been Deprecated.  The argparse module is available as a replacement but only for python >= 2.7.  So a version conditional would be needed for MinGW if the new argparse module were used.

Thanks -- Joey

Comment 10

[(not currently active) Ted Mielczarek]

(In reply to Joey Armstrong [:joey] from comment #9)
> The script is being launched as part of configure -- granted the call was
> not embedded and directly invoked by configure.sh but this is being called
> as a final dependency of the configure target.

When I say "as part of configure", I really do mean "as part of the configure script", not just alongside it from client.mk. I think it makes more sense there.

> One benefit added by the makefile are dependencies.  The script will only be
> launched as needed, when the generated file does not exist or platformKV.py
> is modified.  W/o deps the script must launched incessantly with each build.

We already only run configure as-needed, so I don't think it buys us much to drag in an extra Makefile to do this. If you really wanted to use dependencies, I think it would make more sense to put them directly in client.mk. (But I don't think that's necessary here.)
> > > +#       buffer2 = sys.getwindowsversion() # 0-32 on win3.1, 1-95/98/me, 2-nt/2000/xp/x64, 3-ce
> > > +#       if getattr(sys, 'getwindowsversion', None) is not None:
> > > +#           sys.getwindowsversion()[0] == 6
> > 
> > If you're not going to use this code, just delete it. Leaving it in but
> > commented out is confusing.
> 
> The value is being used:
>   o os_arch_common would be the generic class name 'windows'
>   o os_arch_type set by the block above would be the flavor of windows -
> 7/2000/xp/nt/...

I'm referring to the lines of code that are commented out. Either uncomment them or remove them.

> Edits pending.  I'll start moving these suggestions over into the coding
> style guide to fill out a python section for posterity:
>    http://developer.mozilla.org/En/Mozilla_Coding_Style_Guide

Yeah, sorry, we really should have a Python style guide. I tend to follow most of:
http://www.python.org/dev/peps/pep-0008/ and
http://python.net/~goodger/projects/pycon/2007/idiomatic/handout.html

> I did try platform early on but the function was not quite as portable as
> uname.  MinGW was a problem child in it's own rigt, both platform and uname
> modules were unable to return a usable value.

I've never had that problem. platform.uname()[0] == "Windows". We only support building with MozillaBuild, which includes a standard Windows Python, so you don't have to worry about supporting anything else there.

Comment 11

[Joey Armstrong [:joey]]

Created a new makefile for container use (bug: 623617) that derives many of the values needed from autoconf.mk so platformKV.py is not needed at the moment.  Some hardware values may be needed later on [ cores / processors ] to set a default -j level for make but that can be added later on.