Closed Bug 747597 (install_app.py) Opened 12 years ago Closed 10 years ago

Port XULRunner's --install-app to Python

Categories

(Toolkit Graveyard :: XULRunner, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: WeirdAl, Assigned: WeirdAl)

References

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 8 obsolete files)

9.55 KB, patch
WeirdAl
: review+
Details | Diff | Splinter Review
16.76 KB, patch
WeirdAl
: review+
Details | Diff | Splinter Review
Quoting Bug 741462 comment 1:

I would actually rather we remove --install-app because system XULRunner is obsolete, and for an application developer a python script seems to be a better/simpler alternative.
I'll start by writing this script for Python 2.7, unless someone tells me otherwise.  I figure I need the following:

* Default file locations (copying over key values from the directory service)
* INI parser
* ZIP reader
* Copying directory trees
* Argument validation
* Help console output
* Maybe plistlib for creating Info.plist

The good news is all of these are available with Python already.  I'm undecided on plistlib, as a simple here document will suffice as well.

I'll also probably use the XUL preprocessor to insert an operating system specific Python script into install-app.py (so Mac cruft isn't there on Windows, etc.)  After I get everything in place, then I'll add in build system support.
I've tested this patch on Linux, Mac & Windows, against unzipped folders containing application.ini, and it works.  I have *NOT* tested this against zip archives containing application.ini.

I do a couple more things than the original nsXULAppInstall.js.  For one, I copy the XULRunner binaries into the location specified on DevMo's "Deploying XULRunner" page.  nsXULAppInstall.js didn't do that.  I also enforce the presence of required fields (without validating their values) in application.ini.  Finally, tilde paths were not supported in install locations (what I call "target directories").  So if you tried ~/dist/foo, it just wouldn't work.  The python port fixes that.

On Linux, I get a permissions error when I try to install without a target directory.  This is unsurprising because /usr/local/bin is a privileged location.  I also tested make package and make sdk on this platform, and install-app.py shows up correctly in xulrunner/bin/ on both .tar.bz2 files.

On Mac, there's no application icons support.  I think this is okay because the existing --install-app code didn't support that either.

I have no idea how to write automated tests for this.
Attachment #617279 - Flags: superreview?(benjamin)
Attachment #617279 - Flags: review?(mark.finkle)
Attachment #617280 - Flags: superreview?(benjamin)
Attachment #617280 - Flags: review?(mark.finkle)
Target Milestone: --- → mozilla15
Comment on attachment 617279 [details] [diff] [review]
part 1:  Implement install-app.py as a replacement for xulrunner --install-app.

hrm. I'm not sure that this script does exactly what I want. Ideally this is a script that we ship with the SDK but not the runtime package. It would take a XULRunner app and package it up. Minimally this would be to the current target directory, but it would be nice to also go the final distance and package into an installer or tar.bz2 or .dmg. In any case I can't think of a reason we'd ever want to default to the final install location (Program Files;Applications;usr/local/lib) for this tool, because this is only a tool that a developer would run, not an end-user.

I'm also confused about your use of "expanduser". Normally the shell takes care of tilde expansion before python sees it, so I'm not sure why you need to repeat that procedure here.

It might be nice to make this script into an importable function also, so e.g.:

def installApp(appdir, installdir, appname, gredir):
  ...

if __name__ == '__main__':
  cmds = parser.parse_args()
  appdir, installdir, appname, gredir = validate(cmds)
  installApp(appdir, installdir, appname, gredir)

In particular we should not assume that this script is in the XULRunner install directory and so it shouldn't assume that argv[0] points to the GRE directory.
Attachment #617279 - Flags: superreview?(benjamin)
Attachment #617279 - Flags: superreview-
Attachment #617279 - Flags: review?(mark.finkle)
Attachment #617280 - Flags: superreview?(benjamin)
Attachment #617280 - Flags: superreview+
Attachment #617280 - Flags: review?(mark.finkle)
Let the horse-trading begin.  :)

* Shipping with SDK only:  the SDK has a lot of files in it (.idl, .h, probably xpcshell, etc.) that we shouldn't copy, but my script isn't yet smart enough to know that.  What directories should I copy and what should I exclude?
* Creating installers / dmg / .tar.bz2:  I'd like that to be in a follow-up bug, if you don't mind.  Getting that right (except of course for .tar.bz2) would be more difficult.
* Removing the default installation location: fine by me!
* expanduser: I'll test without it and get back to you.  It should be okay, I think.
* Making it a module:  Good idea, one I had after I posted the patch, but see next bullet point.
* argv[0]:  I'm wondering how to avoid this.  There have to be some criteria for determining where the GRE directory is.  What would those criteria be?
* Reviews:  Are you and Mark (as submodule owner) both okay with just your review on this?  This is an API change, as far as I am concerned, even if it's only for developers.
* Inline documentation:  any suggestions?
Also, since xulrunner on Mac is pretty much useless unless you use install-app, maybe we should just stop building the stock package on Mac:  build only the SDK, I mean.
I've updated the patch and retested against Macintosh.  I believe this patch will work on Linux & Windows, but I haven't had a chance to test that yet.
Attachment #617279 - Attachment is obsolete: true
Attachment #619448 - Flags: review?(benjamin)
Tested on Linux & Windows, and it works fine... so this works on all three major platforms.
Slight problem:  I can't call "import install-app".  The python file has to be renamed to something like install_app.py to be importable.
Alias: install-app.py → install_app.py
bsmedberg: This patch has been in your queue for three weeks.  Any idea when this might be reviewed?
Comment on attachment 619448 [details] [diff] [review]
part 1: Implement install-app.py as a replacement for xulrunner --install-app.

trying to get some traction on reviews
Attachment #619448 - Flags: review?(dtownsend+bugmail)
Target Milestone: mozilla15 → ---
Updated to latest trunk.
Attachment #619448 - Attachment is obsolete: true
Attachment #619448 - Flags: review?(dtownsend+bugmail)
Attachment #619448 - Flags: review?(benjamin)
Attachment #696540 - Flags: review?(dtownsend+bugmail)
Sorry for the bugspam; I left an additional copy of install_app.py in the previous patch.
Attachment #696540 - Attachment is obsolete: true
Attachment #696540 - Flags: review?(dtownsend+bugmail)
Attachment #696541 - Flags: review?(dtownsend+bugmail)
Blocks: 448069
Comment on attachment 696541 [details] [diff] [review]
part 1: Implement install_app.py as a replacement for xulrunner --install-app.

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

::: xulrunner/app/Makefile.in
@@ +161,5 @@
>  	ln -s $(FRAMEWORK_VERSION) $(DIST)/$(FRAMEWORK_NAME).framework/Versions/Current
>  	ln -s Versions/Current/libxpcom.dylib $(DIST)/$(FRAMEWORK_NAME).framework/libxpcom.dylib
>  	ln -s Versions/Current/XUL $(DIST)/$(FRAMEWORK_NAME).framework/XUL
>  	ln -s Versions/Current/xulrunner $(DIST)/$(FRAMEWORK_NAME).framework/xulrunner
> +	$(INSTALL) $< $(DIST)/bin

Seems sort of flaky to assume dependency order here, probably going to want a build-config guy to take a quick look at this though.

@@ +163,5 @@
>  	ln -s Versions/Current/XUL $(DIST)/$(FRAMEWORK_NAME).framework/XUL
>  	ln -s Versions/Current/xulrunner $(DIST)/$(FRAMEWORK_NAME).framework/xulrunner
> +	$(INSTALL) $< $(DIST)/bin
> +
> +GARBAGE += install_app.py install_app.pyc

Why add these?

::: xulrunner/app/install_app.py
@@ +15,5 @@
> +# Platform-specific support
> +# see https://developer.mozilla.org/en/XULRunner/Deploying_XULRunner_1.8
> +if sys.platform.startswith('linux'):
> +    xulrunnerStubName = "xulrunner-stub";
> +    

whitespace

@@ +31,5 @@
> +        return leafName.lower();
> +
> +elif sys.platform == "win32":
> +    xulrunnerStubName = "xulrunner-stub.exe";
> +    

whitespace

@@ +101,5 @@
> +
> +        # Contents/PkgInfo
> +        PkgInfo = open(os.path.join(installDir, "Contents/PkgInfo"), "w+b");
> +        # XXX ajvincent Why this string?  What customizations are possible here?
> +        PkgInfo.write("AAPL????");

Looks unnecessary if you have CFBundleSignature in your Info.plist:
https://developer.apple.com/library/mac/#documentation/MacOSX/Conceptual/BPRuntimeConfig/Articles/ConfigApplications.html

@@ +115,5 @@
> +# End platform-specific support
> +
> +def resolvePath(path):
> +    if (path[0] == "."):
> +        path = os.path.join(os.getcwd(), path);

os.path.join handles relative and absolute paths correctly so you should require that relative paths start with ".". Just join them and you're done

@@ +120,5 @@
> +    return os.path.realpath(path);
> +
> +def requireINIOption(iniparser, section, option):
> +    if not (iniparser.has_option(section, option)):
> +        raise Exception("application.ini must have a " + option + "option under the " +  section + "section");

Nit: space before "option" and "section"

@@ +154,5 @@
> +    return zipApp, iniparser;
> +    pass;
> +
> +def copyGRE(greDir, targetDir):
> +    shutil.copytree(greDir, targetDir);

Pass symlinks=True to save space

@@ +180,5 @@
> +        appName = iniparser.get("App", "Name");
> +    else:
> +        # checkAppINI will get called anyway in installApp().
> +        appName = cmds.appName;
> +    appName = fixLeafName(appName);

I think it would be more consistent to always call checkAppINI here.
Also there are a bunch of characters that could be in the app name but aren't safe for use in file names, does everything fail sanely in that case?

@@ +209,5 @@
> +    parser.add_argument(
> +        "greDir",
> +        action="store",
> +        help="The directory containing the Gecko SDK (usually where this Python script lives)"
> +    );

Can we make this default to the directory containing this script? Also does it need to be the SDK directory or should a runtime package work?
Attachment #696541 - Flags: review?(dtownsend+bugmail) → review-
(In reply to Dave Townsend (:Mossop) from comment #14)

On the build-config:  sure, I'll ask for feedback? on the next patch.

> @@ +163,5 @@
> >  	ln -s Versions/Current/XUL $(DIST)/$(FRAMEWORK_NAME).framework/XUL
> >  	ln -s Versions/Current/xulrunner $(DIST)/$(FRAMEWORK_NAME).framework/xulrunner
> > +	$(INSTALL) $< $(DIST)/bin
> > +
> > +GARBAGE += install_app.py install_app.pyc
> 
> Why add these?

I can't recall exactly whether these show up in the final app or in the dist folder, but in either case I don't want that.

> @@ +180,5 @@
> > +        appName = iniparser.get("App", "Name");
> > +    else:
> > +        # checkAppINI will get called anyway in installApp().
> > +        appName = cmds.appName;
> > +    appName = fixLeafName(appName);
> 
> I think it would be more consistent to always call checkAppINI here.
> Also there are a bunch of characters that could be in the app name but
> aren't safe for use in file names, does everything fail sanely in that case?

Good point on file names; I don't know how to write that verification.  Helpwanted.

Other nits fixed.
Attached patch part 1 in progress (obsolete) — Splinter Review
Seeking feedback help from:
Mossop on filename validation
khuey for build system (see comment 14 please)
Attachment #704419 - Flags: feedback?(khuey)
Attachment #704419 - Flags: feedback?(dtownsend+bugmail)
(In reply to Alex Vincent [:WeirdAl] from comment #15)
> (In reply to Dave Townsend (:Mossop) from comment #14)
> > @@ +180,5 @@
> > > +        appName = iniparser.get("App", "Name");
> > > +    else:
> > > +        # checkAppINI will get called anyway in installApp().
> > > +        appName = cmds.appName;
> > > +    appName = fixLeafName(appName);
> > 
> > I think it would be more consistent to always call checkAppINI here.
> > Also there are a bunch of characters that could be in the app name but
> > aren't safe for use in file names, does everything fail sanely in that case?
> 
> Good point on file names; I don't know how to write that verification. 
> Helpwanted.

I'd suggest against implementing your own. Presumably the attempt to create the directory with an invalid name will fail. All I'm asking is that you check that when that happens the script handles it gracefully (aborts doing anything) and shows an error that is at least semi-useful to the user. The latter might involve catching the error and displaying your own but I don't know what it says currently, try it out
Attachment #704419 - Flags: feedback?(dtownsend+bugmail) → feedback+
(In reply to Dave Townsend (:Mossop) from comment #14)
> Comment on attachment 696541 [details] [diff] [review]
> part 1: Implement install_app.py as a replacement for xulrunner
> --install-app.
> 
> Review of attachment 696541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +154,5 @@
> > +    return zipApp, iniparser;
> > +    pass;
> > +
> > +def copyGRE(greDir, targetDir):
> > +    shutil.copytree(greDir, targetDir);
> 
> Pass symlinks=True to save space

If you symlink here then the resulting .app isn't really self contained and can't be distributed. Shouldn't this default to a real copy of the .framework and maybe support symlinks as an option to save space/time for app development?
(In reply to Dave Townsend (:Mossop) from comment #17)
> (In reply to Alex Vincent [:WeirdAl] from comment #15)
> > (In reply to Dave Townsend (:Mossop) from comment #14)
> > > @@ +180,5 @@
> > > > +        appName = iniparser.get("App", "Name");
> > > > +    else:
> > > > +        # checkAppINI will get called anyway in installApp().
> > > > +        appName = cmds.appName;
> > > > +    appName = fixLeafName(appName);
> > > 
> > > I think it would be more consistent to always call checkAppINI here.
> > > Also there are a bunch of characters that could be in the app name but
> > > aren't safe for use in file names, does everything fail sanely in that case?
> > 
> > Good point on file names; I don't know how to write that verification. 
> > Helpwanted.
> 
> I'd suggest against implementing your own. Presumably the attempt to create
> the directory with an invalid name will fail. All I'm asking is that you
> check that when that happens the script handles it gracefully (aborts doing
> anything) and shows an error that is at least semi-useful to the user. The
> latter might involve catching the error and displaying your own but I don't
> know what it says currently, try it out

This is going to sound stupid, but:  what sort of characters are illegal for file names?

My apologies for abandoning this bug for so long...
(In reply to Alex Vincent [:WeirdAl] from comment #19)
> (In reply to Dave Townsend (:Mossop) from comment #17)
> > (In reply to Alex Vincent [:WeirdAl] from comment #15)
> > > (In reply to Dave Townsend (:Mossop) from comment #14)
> > > > @@ +180,5 @@
> > > > > +        appName = iniparser.get("App", "Name");
> > > > > +    else:
> > > > > +        # checkAppINI will get called anyway in installApp().
> > > > > +        appName = cmds.appName;
> > > > > +    appName = fixLeafName(appName);
> > > > 
> > > > I think it would be more consistent to always call checkAppINI here.
> > > > Also there are a bunch of characters that could be in the app name but
> > > > aren't safe for use in file names, does everything fail sanely in that case?
> > > 
> > > Good point on file names; I don't know how to write that verification. 
> > > Helpwanted.
> > 
> > I'd suggest against implementing your own. Presumably the attempt to create
> > the directory with an invalid name will fail. All I'm asking is that you
> > check that when that happens the script handles it gracefully (aborts doing
> > anything) and shows an error that is at least semi-useful to the user. The
> > latter might involve catching the error and displaying your own but I don't
> > know what it says currently, try it out
> 
> This is going to sound stupid, but:  what sort of characters are illegal for
> file names?
> 
> My apologies for abandoning this bug for so long...

At least on windows these characters are not allowed: \ / : * ? " < > |
Both patches have bitrotted significantly...
Attachment #696541 - Attachment is obsolete: true
Attachment #704419 - Attachment is obsolete: true
Rebased to most recent code.
Attachment #617280 - Attachment is obsolete: true
> At least on windows these characters are not allowed: \ / : * ? " < > |

On Linux, a null character is also disallowed.  I've altered install_app.py so it explicitly throws when appName contains an illegal character for Windows or Linux.  Also, I moved the validation of appName & others into the installApp() method, because in earlier iterations the validation was on command-line arguments only.
Attachment #825138 - Flags: review?(dtownsend+bugmail)
Attachment #825140 - Flags: review?(benjamin)
Attachment #825140 - Flags: review?(benjamin) → review+
Comment on attachment 825138 [details] [diff] [review]
part 1: Implement install_app.py as a replacement for xulrunner --install-app.

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

This is generally fine. A few things that it might be worth changing but I'm happy not to see it again

::: xulrunner/app/install_app.py
@@ +6,5 @@
> +
> +# Min version of python is 2.7
> +import sys
> +if ((sys.version_info.major < 2) or (sys.version_info.minor < 7)):
> +    raise Exception("You need to use Python version 2.7 or higher")

So this fails on Python 3.0 but not 3.7. Maybe just change the major test to !=2

@@ +18,5 @@
> +    xulrunnerStubName = "xulrunner-stub"
> +
> +    def installApp(appLocation, installDir, appName, greDir):
> +        appName = validateArguments(appLocation, installDir, appName, greDir)
> +        zipApp, iniParser = checkAppINI(appLocation)

can we just have validateArguments return these rather than calling this a second time?

@@ +46,5 @@
> +                     os.path.join(installDir, appName))
> +        copyGRE(greDir, os.path.join(installDir, "xulrunner"))
> +
> +    def fixLeafName(leafName):
> +        return leafName

If you appended ".exe" here then the installApp for windows and linux would be identical and could just be shared. I might call it makeAppName instead.

@@ +112,5 @@
> +    raise Exception("This operating system isn't supported for install_app.py yet!")
> +# End platform-specific support
> +
> +def resolvePath(path):
> +    path = os.path.join(os.getcwd(), path)

From testing locally I think this is unnecessary, realpath seems to expand relative paths already

@@ +152,5 @@
> +    pass
> +
> +def copyGRE(greDir, targetDir):
> +    shutil.copytree(greDir, targetDir, symlinks=True)
> +    pass

pass is unnecessary

@@ +155,5 @@
> +    shutil.copytree(greDir, targetDir, symlinks=True)
> +    pass
> +
> +def extractArguments(cmds):
> +    return cmds.appLocation, cmds.installDir, cmds.appName, cmds.greDir

Unused

@@ +220,5 @@
> +    parser.add_argument("--version", action="version", version="%(prog)s 1.0")
> +
> +    # The command code.
> +    cmds = parser.parse_args()
> +    installApp(cmds.appLocation, cmds.installDir, cmds.appName, cmds.greDir)

If this throws an exception do you want to delete installDir to clean up?
Attachment #825138 - Flags: review?(dtownsend+bugmail) → review+
Attachment #825140 - Attachment is obsolete: true
Attachment #8357650 - Flags: review+
Thanks, Mossop, for all your suggestions, which have been incorporated.  There was also a syntax error which I fixed after the patch had been written:

-        raise Exception("XULRunner stub executable not found: " + xulrunnerStubPath);
+        raise Exception("XULRunner stub executable not found: " + os.path.join(greDir, xulrunnerStubName));

Part 2 had to be resubmitted due to bitrot.
Keywords: checkin-needed
Keywords: dev-doc-needed
Depends on: 958814
I'm getting an error running install_app.py:

Traceback (most recent call last):
  File "install_app.py", line 221, in <module>
    handleCommandLine()
  File "install_app.py", line 215, in handleCommandLine
    except exn:
NameError: global name 'exn' is not defined

Should I be running it from ./bin/install_app.py or ./XUL.framework/Versions/29.0.1/install_app.py?
I'm also getting the same error as Mike on install_app.py. Looking at the source, the issue appears to be the Python script crashing when trying to catch another exception (`exn` is the variable name). However, the Python syntax appears to be incorrect.

When I fixed the syntax by removing `exn`, the undefined global name, I saw the deeper issue in my case, which was that the bundle directory already existed (though your error may have a different underlying cause). After addressing that, I was able to run the script.
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.