776611 - packager.mk: manifest files for {PACK,UNPACK}_OMNIJAR

Status: RESOLVED WONTFIX

(Firefox Build System :: General, defect)

Description

[Joey Armstrong [:joey]]

omnijar commands: move shell commands that manipulate manifest files and generate {binary,chrome}.manifest a python script/module and replace them with a call to the script.

A temp file can be created in the components directory in place of $(pwd)/binary.manifest + mv b.m components/.

Add unit tests as needed

Comment 1

[Joey Armstrong [:joey]]

Attachment: packager.mk - migrate omnijar manifest logic into a python module.

Comment 2

[Joey Armstrong [:joey]]

Comment on attachment 645397 [details] [diff] [review]
packager.mk - migrate omnijar manifest logic into a python module.

Moved manifest manipulation logic out of packager.mk::PACK_OMNIJAR and into a python module manifest.py.  Added a front end script omnijar.py for calling packager.mk utility functions as logic is rewritten in python.  Replaced shell for loop/grep/sed, etc with calls to omnijar.py.

unit tests added for the new module.

Fixed/replaced a hardcoded 'make -C' reference in packager.mk

Presence of --manifest-create & --manifest-revert are used by {PACK,UNPACK}_OMNIJAR to manipulate manifests and create binary.manifest.

Flags can be omitted by {PACK,UNPACK}_OMNIJAR_WEBAPP_RUNTIME when the time comes.

Try job and jarfile comparison with a clean sandbox still pending.

Comment 3

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

Comment on attachment 645397 [details] [diff] [review]
packager.mk - migrate omnijar manifest logic into a python module.

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

A good start, but I'd like to see some changes.

I don't think we need a separate config file.  Just make the values in the config file the defaults for --verbose and --debug.

::: toolkit/mozapps/installer/omnijar.py
@@ +84,5 @@
> +    # import local modules
> +    basedir = os.path.dirname(os.path.abspath(__file__))
> +    utils = "%s/utils" % (basedir)
> +    sys.path.insert(0, utils)
> +    import manifest

I think this should be set up using pythonpath or virtualenv or whatever we're using these days, but not in the script itself.

@@ +96,5 @@
> +
> +    if opts.manifest_create:
> +        manifest.binaryManifest(opts.manifest_binary, create=True)
> +
> +    if opts.manifest_revert:

Please enforce that manifest-create and manifest-revert are not used together.

@@ +104,5 @@
> +        if not opts.manifest_binary:
> +            raise Exception("--manifest-binary is required")
> +        manifest.genChromeManifest(opts.manifest_binary, opts.manifest_chrome)
> +
> +    sys.exit(rc)

Just sys.exit(0)?  You don't ever assign anything else to rc.

::: toolkit/mozapps/installer/packager.mk
@@ +505,5 @@
> +  $(RM) -v $(OMNIJAR_NAME) && \
> +  $(omnijar_py) \
> +    --manifest-create \
> +    --manifest-binary $(manifest_binary) \
> +	$(NULL) && \

Unnecessary tab.

::: toolkit/mozapps/installer/utils/makefile
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +PYTHON ?= python

Please use the standard makefile format here.

::: toolkit/mozapps/installer/utils/manifest.py
@@ +44,5 @@
> +            data = mani.readlines()
> +            mani.close()
> +
> +            havebin = False
> +            for line in data:

no need for the temporary.  Just do 'for line in mani.readlines():'

@@ +63,5 @@
> +
> +            if not havebin:
> +                continue
> +
> +            bin = open(manifestB, 'a')

with open(manifestB, 'a') as bin: ?

@@ +136,5 @@
> +    logging.debug("pwd=%s, tempdir=%s" % (here, work))
> +
> +    ##########################
> +    ## test: binaryManifest
> +    ##########################

I think the test should be in a separate file.

Comment 4

[Joey Armstrong [:joey]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> packager.mk - migrate omnijar manifest logic into a python module.

Thanks - these two would happen eventually after more modules have been created but I can add the unit test infra now if needed.

> ::: toolkit/mozapps/installer/utils/makefile
> @@ +3,5 @@
> > +# This Source Code Form is subject to the terms of the Mozilla Public
> > +# License, v. 2.0. If a copy of the MPL was not distributed with this
> > +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> > +
> > +PYTHON ?= python
> 
> Please use the standard makefile format here.

> > +    logging.debug("pwd=%s, tempdir=%s" % (here, work))
> > +
> > +    ##########################
> > +    ## test: binaryManifest
> > +    ##########################
> 
> I think the test should be in a separate file.

Comment 5

[Joey Armstrong [:joey]]

> packager.mk - migrate omnijar manifest logic into a python module.
>
> ::: toolkit/mozapps/installer/utils/manifest.py
> 
> no need for the temporary.  Just do 'for line in mani.readlines():'

The manifest files were small to cache in memory.  Updating but the edit will separate open() & close() by a screen or two of the I/O body.


> @@ +63,5 @@
> > +            bin = open(manifestB, 'a')
> 
> with open(manifestB, 'a') as bin: ?

The assignment was written using with initially like most of the other open calls but python was not a happy camper about syntax for this specific call for some reason.  I'll check on it again.

Comment 6

[Joey Armstrong [:joey]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #3)
> Comment on attachment 645397 [details] [diff] [review]
> packager.mk - migrate omnijar manifest logic into a python module.
> 
> ::: toolkit/mozapps/installer/omnijar.py
> @@ +84,5 @@
> > +    # import local modules
> > +    basedir = os.path.dirname(os.path.abspath(__file__))
> > +    utils = "%s/utils" % (basedir)
> > +    sys.path.insert(0, utils)
> > +    import manifest
> 
> I think this should be set up using pythonpath or virtualenv or whatever
> we're using these days, but not in the script itself.

Breaking up the program/module (versioned as a set) seems dangerous.
An alternate path might allow loading modules from an unexpected directory.
Is there a benefit for delegating -vs- deriving the include path for local modules ?

Comment 7

[(not currently active) Ted Mielczarek]

(In reply to Joey Armstrong [:joey] from comment #6)
> Breaking up the program/module (versioned as a set) seems dangerous.
> An alternate path might allow loading modules from an unexpected directory.
> Is there a benefit for delegating -vs- deriving the include path for local
> modules ?

Loading code from modules is well-understood and standard practice for Python. We have a known set of include paths, so this isn't an issue. Once bug 776046 finishes landing, you will be able to import any .py file from config/ or build/ as a Python module.

Comment 8

[Joey Armstrong [:joey]]

Attachment: packager.mk - migrate omnijar manifest logic into a python module.

Comment 9

[Joey Armstrong [:joey]]

> I don't think we need a separate config file.  Just make the values in the config file the defaults for --verbose and --debug.

Config file removed.  A little less convenient to use for enabling try job debugging but --debug can be hardcoded in packager.mk when needed.


> I think this should be set up using pythonpath or virtualenv or whatever we're using these days, but not in the script itself.

utils/ include path now set by pythonpath


> Please enforce that manifest-create and manifest-revert are not used together.

Test added to complain about conflicting arguments.  A case for unpack=>do-stuff=>repack exists within omni targets but not all intermediate logic has moved from packager.mk into omnijar.py.

An explicit flag or ordered processing arguments should be able to handle this when the time comes.


> @@ +104,5 @@
> > +        if not opts.manifest_binary:
> > +    sys.exit(rc)
> 
> Just sys.exit(0)?  You don't ever assign anything else to rc.

manifest content is the only logic moved out of packager.mk.  More will be moved later with other bugs.  Exit status will need to propagate across functions later but can defer adding rc assignments until the calls are in place.


> ::: toolkit/mozapps/installer/utils/makefile
> 
> Please use the standard makefile format here.

Makefile moved into a test directory and infra added to allow running any unit tests found there.


> @@ +63,5 @@
> > +            bin = open(manifestB, 'a')
> 
> with open(manifestB, 'a') as bin: ?

A future import was needed to keep python 2.5 happy:
   from __future__ import with_statement


> @@ +136,5 @@
> > +    logging.debug("pwd=%s, tempdir=%s" % (here, work))
> > +
> > +    ##########################
> > +    ## test: binaryManifest
> > +    ##########################
> 
> I think the test should be in a separate file.

Moved into utils/test where Makefile.in can find it.

Comment 10

[Jim Mathies [:jimm]]

ping khuey - hate to be a nag but we have a q3 goal target of nightlies off elm and this and bug is key to getting there.

Comment 11

[Joey Armstrong [:joey]]

Try job results: https://tbpl.mozilla.org/?tree=Try&rev=5c2a570b6368

Comment 12

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

Comment on attachment 647297 [details] [diff] [review]
packager.mk - migrate omnijar manifest logic into a python module.

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

You didn't address all my comments from last time ...

::: toolkit/mozapps/installer/omnijar.py
@@ +72,5 @@
> +    # import local modules
> +    basedir = os.path.dirname(os.path.abspath(__file__))
> +    utils = "%s/utils" % (basedir)
> +    sys.path.insert(0, utils)
> +    import manifest

Again, we shouldn't manually munge sys.path like this.

Comment 13

[Joey Armstrong [:joey]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #12)
> Comment on attachment 647297 [details] [diff] [review]
> packager.mk - migrate omnijar manifest logic into a python module.
> 
> Review of attachment 647297 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You didn't address all my comments from last time ...
> 
> ::: toolkit/mozapps/installer/omnijar.py
> @@ +72,5 @@
> > +    # import local modules
> > +    basedir = os.path.dirname(os.path.abspath(__file__))
> > +    utils = "%s/utils" % (basedir)
> > +    sys.path.insert(0, utils)
> > +    import manifest
> 
> Again, we shouldn't manually munge sys.path like this.

I can remove it but setting the include path externally will force having to run tests beneath the $(obj) directory.  Currently they can be run as test/check-something.sh.

Comment 14

[Mike Hommey [:glandium]]

$objdir/_virtualenv/bin/python test/check-something.py

Comment 15

[Joey Armstrong [:joey]]

(In reply to Mike Hommey [:glandium] from comment #14)
> $objdir/_virtualenv/bin/python test/check-something.py

Yes was mainly trying to avoid typing more than ./

Actually pythonpath was set properly in Makefile.in, the derived path just needs to be removed from the unit test.

Comment 16

[Joey Armstrong [:joey]]

Attachment: packager.mk - migrate omnijar manifest logic into a python module.

Comment 17

[Joey Armstrong [:joey]]

Comment on attachment 649344 [details] [diff] [review]
packager.mk - migrate omnijar manifest logic into a python module.

Removed derived path logic from the manifest.tpy unit test.  Script will now rely on the include path being set by make or an external wrapper.

Comment 18

[Joey Armstrong [:joey]]

Comment on attachment 649344 [details] [diff] [review]
packager.mk - migrate omnijar manifest logic into a python module.

need to re-apply the omnijar pythonpath patch

Comment 19

[Joey Armstrong [:joey]]

Attachment: packager.mk - migrate omnijar manifest logic into a python module.

Comment 20

[Joey Armstrong [:joey]]

Comment on attachment 649731 [details] [diff] [review]
packager.mk - migrate omnijar manifest logic into a python module.

Removed lingering reference to os.path.insert() within the unit test.
module include path set everywhere with a wrapped call to python path from the makefile.

Comment 21

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

Comment on attachment 649731 [details] [diff] [review]
packager.mk - migrate omnijar manifest logic into a python module.

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

Holding off on a final r+ until the question about '-u' is answered, but it looks good.

::: toolkit/mozapps/installer/omnijar.py
@@ +15,5 @@
> +import logging
> +
> +import manifest
> +
> +def getopts(): 

Extra white space at the end of line.

::: toolkit/mozapps/installer/packager.mk
@@ +497,5 @@
>    res/MainMenu.nib/\* \
>    $(NULL)
>  
> +omnijar_py = $(PYTHON) \
> +  -u $(topsrcdir)/config/pythonpath.py \

Why do you need the '-u' option here (for unbuffered output)?

@@ +509,4 @@
>  PACK_OMNIJAR	= \
> +  $(RM) -v $(OMNIJAR_NAME) && \
> +  $(omnijar_py) \
> +    --debug \

Please remove the --debug switches before landing.

@@ +516,5 @@
>    $(ZIP) -r9m $(OMNIJAR_NAME) $(OMNIJAR_FILES) -x $(NON_OMNIJAR_FILES) && \
>    $(GENERATE_CACHE) && \
>    $(OPTIMIZE_JARS_CMD) --optimize $(JARLOG_DIR_AB_CD) ./ ./ && \
> +  $(omnijar_py) \
> +    --debug \

here

@@ +525,5 @@
>  UNPACK_OMNIJAR	= \
>    $(OPTIMIZE_JARS_CMD) --deoptimize $(JARLOG_DIR_AB_CD) ./ ./ && \
>    $(UNZIP) -o $(OMNIJAR_NAME) && \
> +  $(omnijar_py) \
> +    --debug \

and here too.

::: toolkit/mozapps/installer/utils/manifest.py
@@ +64,5 @@
> +                continue
> +
> +            with open(manifestB, 'a') as bin:
> +                bin.writelines("%s\n" % item for item in binary)
> +                bin.close()

No need for an explicit .close().  'with' does that for you.

@@ +74,5 @@
> +                txt.close()
> +
> +            shutil.move(tmp, fyl)
> +
> +        mani.close()

No need for the explicit close here either.

@@ +90,5 @@
> +
> +    # todo: verify existence, file removed earlier by zip -m
> +    with open(manifestC, 'w') as chrome:
> +        chrome.write("manifest %s\n" % (manifestB))
> +        chrome.close()

No need for the close here either.

@@ +95,5 @@
> +
> +def main():
> +    """
> +    """
> +    sys.exit(0)

No need for empty docstring comments.

Comment 22

[Joey Armstrong [:joey]]

 > Holding off on a final r+ until the question about '-u' is answered, but it
> looks good.
> 
> ::: toolkit/mozapps/installer/packager.mk
> @@ +497,5 @@
> >    res/MainMenu.nib/\* \
> >    $(NULL)
> >  
> > +omnijar_py = $(PYTHON) \
> > +  -u $(topsrcdir)/config/pythonpath.py \
> 
> Why do you need the '-u' option here (for unbuffered output)?

-u was introduced by copying an existing pythonpath.py line it was not an intentional addition.  I'll remove it with the next patch upload.

THanks

Comment 23

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

Comment on attachment 649731 [details] [diff] [review]
packager.mk - migrate omnijar manifest logic into a python module.

Ok, r=me with -u removed.

Comment 24

[Joey Armstrong [:joey]]

Attachment: packager.mk - migrate omnijar manifest logic into a python module.

Comment 25

[Joey Armstrong [:joey]]

Comment on attachment 652203 [details] [diff] [review]
packager.mk - migrate omnijar manifest logic into a python module.

r=khuey

Removed --debug flag from omnijar.py.
Removed -u python/prefix arg from pythonpath.py.
Removed explicit .close() calls in favor of builtin with statement closures.
trim whitespace from getopts() def line.

Comment 26

[Joey Armstrong [:joey]]

try results https://tbpl.mozilla.org/?tree=Try&rev=ba9b9e2d913e