570689 - Convert preprocess-locale.pl to a python script

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

[Robert Strong (they/them - no direct email)]

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/make-installremoves.pl

http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl

The following files should likely be removed... need to make sure there are no consumers in mc and cc

Old install.js support
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/make-installjsremoves.pl

Old stub and wizard support?
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/makecfgini.pl
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/makeinstallini.pl
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/CFGParser.pm

Comment 1

[Robert Strong (they/them - no direct email)]

Attachment: patch 1 - remove unused files (checked in)

Comment 2

[Robert Strong (they/them - no direct email)]

Comment on attachment 449916 [details] [diff] [review]
patch 1 - remove unused files (checked in) 

File removal patch pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/4762173104c4

Comment 3

[Robert Strong (they/them - no direct email)]

I removed the following in bug 367539
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/make-installremoves.pl

Now all that is left is to convert the following to python which I am almost done with
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/installer/windows/nsis/preprocess-locale.pl

Comment 4

[Robert Strong (they/them - no direct email)]

Changing summary since there is now only one perl script that needs to be converted

Comment 5

[Robert Strong (they/them - no direct email)]

Attachment: patch in progress

Tested with the ja and fa locales... this cleans things up quite a bit

Comment 6

[Robert Strong (they/them - no direct email)]

Attachment: patch - convert preprocess-locale.pl to python

Turns out that NSIS files that are not localized don't need to be converted to utf16le

Comment 7

[Robert Strong (they/them - no direct email)]

Attachment: 3. mozilla-central cleanup patch after comm-central is updated

Comment 8

[Robert Strong (they/them - no direct email)]

Attachment: 2. Thunderbird patch

Phil, this changes it so Thunderbird uses the python script and it removes the iconv requirement for the installer though I didn't update configure.in... perhaps someone can deal with that in a separate bug? Turns out there is no need to convert any of the files to UTF-16LE except for the locale files and that is done in the python script.

Comment 9

[Robert Strong (they/them - no direct email)]

Attachment: 2. SeaMonkey patch

Callek, this changes it so SeaMonkey uses the python script. It doesn't remove the
iconv requirement for SeaMonkey since SeaMonkey's Makefile.in converts license.txt to UTF-16LE... it also adds utf16-le-bom.bin to SeaMonkey since it will be the only one using that file.

Comment 10

[Robert Strong (they/them - no direct email)]

Simon, the changes in this bug are going to break the Sunbird installer. Please let me know if you want me to fix it or not.

Comment 11

[Robert Strong (they/them - no direct email)]

Attachment: 2. SeaMonkey patch rev2

Callek, turns out that the license displays fine as utf-8 so I don't believe it is necessary to add the bom file... leaving the other patch up with review and I'll let you choose.

Comment 12

[Robert Strong (they/them - no direct email)]

Attachment: 2. Sunbird patch

Simon, in case you want the Sunbird patch

Comment 13

[Robert Strong (they/them - no direct email)]

Comment on attachment 453626 [details] [diff] [review]
2. SeaMonkey patch rev2

bah... I take that back. I have the version of NSIS that will be in the next MozillaBuild and when I forced it back to NSIS 2.33 the license page was displayed incorrectly

Comment 14

[Justin Wood (:Callek)]

(In reply to comment #10)
> Simon, the changes in this bug are going to break the Sunbird installer. Please
> let me know if you want me to fix it or not.

If you choose to fix sunbird I'll review for you (I was granted abil to r+ Sunbird packaging/installer/buildconfig too). But Sunbird is tier_3 vs Lighting as a tier_1 so if it breaks no-one will whine about it on the Calendar team. We just have not explicitly killed that config yet.

Comment 15

[Justin Wood (:Callek)]

Comment on attachment 453515 [details] [diff] [review]
2. SeaMonkey patch

I'm not about to block _this_ patch for it. but if SeaMonkey will remain the only thing requiring iconv.

Can you define something like |HOST_NEEDS_ICONV| (or something; I'm not attached to the name) inside suite's confvars.sh and wrap the c-c configure.in MOZ_PATH_PROGS check with that? Along with a comment stating what version of NSIS we can drop this use with (as your alt patch was to do; you can just comment with the MozBuild.next NSIS ver if you want).

Comment 16

[Robert Strong (they/them - no direct email)]

I'm working on a fix so the requirement for iconv will be removed. The rest of the patch will remain the same.

Comment 17

[Justin Wood (:Callek)]

Comment on attachment 453509 [details] [diff] [review]
2. Thunderbird patch

Since I already taught myself to understand this changed code; stealing review and r=Callek

Comment 18

[Justin Wood (:Callek)]

Comment on attachment 453628 [details] [diff] [review]
2. Sunbird patch

>+BRANDING_FILES = \
> 	branding.nsi \
> 	license.txt \

Less of a worry on my end for Sunbird; but while you're here can you verify one way or the other if it needs a license.txt iconv requirement like SeaMonkey? if so can you port over the SeaMonkey line that uses it (and fork the .bin you moved to SeaMonkey to the dir structure under calendar/ please).

You may carry forward my r+ even if thats needed.

Comment 19

[Justin Wood (:Callek)]

Comment on attachment 453473 [details] [diff] [review]
patch - convert preprocess-locale.pl to python

>+
>+if len(sys.argv) == 5:
>+    preprocess_locale_files()

nit wrap in:

|if __name__ == '__main__':|

also in the argv length test
* Test for a --help type of string and print a brief usage instruction?
* Also raise an error "Not Enough Arguments" or something for a meaningful failure case.

Comment 20

[Robert Strong (they/them - no direct email)]

Attachment: 1. patch - convert preprocess-locale.pl to python rev2

Callek, thanks for the feedback... this addresses your comments

Comment 21

[Robert Strong (they/them - no direct email)]

Attachment: 2. SeaMonkey patch rev2

This removes the requirement for iconv

Comment 22

[Justin Wood (:Callek)]

(In reply to comment #13)
> (From update of attachment 453626 [details] [diff] [review])
> bah... I take that back. I have the version of NSIS that will be in the next
> MozillaBuild and when I forced it back to NSIS 2.33 the license page was
> displayed incorrectly

(In reply to comment #21)
> Created an attachment (id=453645) [details]
> 2. SeaMonkey patch rev2
> 
> This removes the requirement for iconv

Huh, does SM need or not need iconv for license.txt to display correctly when built on NSIS that MozillaBuild ships as of today?

Comment 23

[Robert Strong (they/them - no direct email)]

It doesn't if it is converted to utf-16-le by preprocess-locale.py

Comment 24

[Robert Strong (they/them - no direct email)]

Attachment: 2. Sunbird patch

btw: if you'd like me to update Sunbird's installer and you can review the changes I can do so... it shouldn't take me more than a half an hour to create a patch when I have some spare time.

Comment 25

[Justin Wood (:Callek)]

Comment on attachment 453645 [details] [diff] [review]
2. SeaMonkey patch rev2

Ahh in that case, please make a new patch for the c-c configure.in for the mirror of the m-c configure.in change you did.

Comment 26

[Robert Strong (they/them - no direct email)]

Attachment: 2. comm-central configure patch

Comment 27

[Simon Paquet [:sipaq]]

Comment on attachment 453647 [details] [diff] [review]
2. Sunbird patch

Looks good.
r=sipaq

Comment 28

[Robert Strong (they/them - no direct email)]

Ted ping

Comment 29

[(not currently active) Ted Mielczarek]

Sorry, Summit+Vacation back to back let my review queue get way behind.

Comment 30

[(not currently active) Ted Mielczarek]

Comment on attachment 453644 [details] [diff] [review]
1. patch - convert preprocess-locale.pl to python rev2

>diff --git a/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py b/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py
>@@ -0,0 +1,123 @@
>+# make-installremoves.py

Your comment doesn't match the filename :) Could you add a short comment describing the purpose of the script beneath the license header?

As a general comment, it'd be useful if you added a docstring comment to each function with a short description of its purpose.

>+def get_locale_strings(path, prefix, middle, add_cr):
>+    output = ''
>+    fp = open(path, "r")
>+    for line in fp:
>+        line = line.strip()
>+        if line == "" or line[0] == "#":
>+            continue
>+
>+        [name, value] = line.split("=", 1)

You probably just want name, value (which gives you a tuple) instead of a list here. (They both work, but tuples are more Pythonic, I guess.)

>+def preprocess_locale_files():
>+        moz_dir = sys.argv[1]    # root dir containing the toolkit source

You've got some inconsistent indentation here. Stick with 4-space. Also, I'd prefer it if you passed the bits of sys.argv you need into this function as named parameters. (Same with the other top-level functions.)

>+        # Set the language ID to 0 to make this locale the default locale. An
>+        # actual ID will need to be used to create a multi-language installer
>+        # (e.g. for CD distributions, etc.).
>+        lang_id = "0"
>+        rtl = "-"
>+
>+        # Check whether the locale is right to left from locales.nsi.
>+        fp = open(join(moz_dir, "toolkit/mozapps/installer/windows/nsis/locales.nsi"), "r")
>+        for line in fp:
>+            line = line.strip()
>+            if line == "!define " + ab_cd + "_rtl":
>+                rtl = "RTL"
>+                break
>+
>+        fp.close()
>+
>+        # Create the main NSIS language file with RTL for right to left locales
>+        # along with the default codepage, font name, and font size represented
>+        # by the '-' character.
>+        fp = open_utf16le_file(join(config_dir, "baseLocale.nlf"))
>+        fp.write(unicode("# Header, don't edit\n" +
>+                         "NLF v6\n" +
>+                         "# Start editing here\n" +
>+                         "# Language ID\n" +
>+                         lang_id + "\n" +
>+                         "# Font and size - dash (-) means default\n" +
>+                         "-\n" +
>+                         "-\n" +
>+                         "# Codepage - dash (-) means ANSI code page\n" +
>+                         "-\n" +
>+                         "# RTL - anything else than RTL means LTR\n" +
>+                         rtl + "\n").encode("utf-16-le"))

If you're going to do string interpolation like this, I prefer if you use the % operator. Also, if you need multi-line strings, you can use triple-quoted strings. Finally, you can write a Unicode literal in Python like u"", so you could write this statement more like:
fp.write(u"""# Header, don't edit
NLF v6
# Start editing here
# Language ID
%s
# ...
""" % (lang_id, ... )

>+if __name__ == '__main__':
>+    if len(sys.argv) == 3:
>+        convert_utf8_utf16le()
>+    elif len(sys.argv) == 5:
>+        preprocess_locale_files()
>+    else:
>+        sys.stderr.write("Invalid number of arguments\n")
>+        sys.exit(1)

I'd prefer if the script took a first argument indicating what function to perform rather than detecting the number of arguments. I think that will make it clearer when reading the Makefile invocations. Something like:
preprocess-locale.py --convert-utf8-utf16le a b c
or
preprocess-loca.py convert a b c

would be fine.

r=me with those changes.

Comment 31

[Robert Strong (they/them - no direct email)]

(In reply to comment #30)
>...
> If you're going to do string interpolation like this, I prefer if you use the %
> operator. Also, if you need multi-line strings, you can use triple-quoted
> strings. Finally, you can write a Unicode literal in Python like u"", so you
> could write this statement more like:
I'd prefer to use unicode("") since u"" is deprecated in Python3K and I don't know how to call .encode("utf-16-le") on u"" (is it possible?). I'll take care of the rest though at times I'm torn between indenting the multiline strings vs using triple quotes... such is python.

Comment 32

[(not currently active) Ted Mielczarek]

unicode("foo") and u"foo" are equivalent, so you can just do u"foo".encode("utf-16-le"). Also, AFAIK unicode() is also removed in Python 3, since str() gives you a unicode string (and string literals are unicode by default), so I'm not sure it matters either way.

Comment 33

[Justin Wood (:Callek)]

(In reply to comment #32)
> Also, AFAIK unicode() is also removed in Python 3,

Well if that is true [did not check] then unicode() still might be the better choice, since when we start to migrate our code to py3 we can easily define unicode() as a function, accepting a string, and returning a string.

Where u"" would error.

Comment 34

[Robert Strong (they/them - no direct email)]

Attachment: 2. patch - convert preprocess-locale.pl to python rev3 (checked in)

Ted, would you mind taking another look at this? I ended up having to wrap u" in parens to get .encode to work when using the % operator as follows.

fp.write((u"""# Header, don't edit
...
%s
""" % (lang_id, rtl)).encode("utf-16-le"))

Comment 35

[(not currently active) Ted Mielczarek]

Comment on attachment 460994 [details] [diff] [review]
2. patch - convert preprocess-locale.pl to python rev3 (checked in)

Looks good, thanks!

Comment 36

[Robert Strong (they/them - no direct email)]

Comment on attachment 460994 [details] [diff] [review]
2. patch - convert preprocess-locale.pl to python rev3 (checked in)

Drivers, IMO this patch significantly cleans up the installer build process (conversion from perl to python, removal of files, simplification of Makefiles, etc.). Not critical for Firefox 2.0 but I believe it is still early enough in the cycle to get this cleanup done.

Comment 37

[Robert Strong (they/them - no direct email)]

Comment on attachment 453505 [details] [diff] [review]
3. mozilla-central cleanup patch after comm-central is updated

Drivers, this is the final patch to land after the comm-central patches have landed.

Comment 38

[Robert Strong (they/them - no direct email)]

Comment on attachment 460994 [details] [diff] [review]
2. patch - convert preprocess-locale.pl to python rev3 (checked in)

Patch 2 pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/e1161df6af19

Comment 39

[Robert Strong (they/them - no direct email)]

Attachment: 2. Thunderbird patch updated (checked in)

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/666c1eefac5c

Comment 40

[Robert Strong (they/them - no direct email)]

Attachment: 2. Sunbird patch updated (checked in)

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/c8f4cc86dda2

Comment 41

[Robert Strong (they/them - no direct email)]

Attachment: 2. SeaMonkey patch updated (checked in)

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/c05c5d941d2b

Comment 42

[Robert Strong (they/them - no direct email)]

Attachment: 2. configure for comm-central patch updated (checked in)

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/2f35246bf41f

Comment 43

[Robert Strong (they/them - no direct email)]

Attachment: 3. mozilla-central cleanup patch updated (checked in)

Pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/71d27f203ef5