Closed Bug 570689 Opened 10 years ago Closed 10 years ago

Convert preprocess-locale.pl to a python script

Categories

(Toolkit :: NSIS Installer, defect)

x86
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: robert.strong.bugs, Assigned: robert.strong.bugs)

References

Details

Attachments

(8 files, 10 obsolete files)

21.25 KB, patch
ted
: review+
Details | Diff | Splinter Review
7.49 KB, patch
ted
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
16.54 KB, patch
ted
: review+
benjamin
: approval2.0+
Details | Diff | Splinter Review
4.24 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
4.65 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
4.58 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
1.25 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
8.30 KB, patch
robert.strong.bugs
: review+
Details | Diff | Splinter Review
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #449916 - Flags: review?(ted.mielczarek)
Attachment #449916 - Flags: review?(ted.mielczarek) → review+
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
Attachment #449916 - Attachment description: patch 1 - remove unused files → patch 1 - remove unused files (checked in)
Changing summary since there is now only one perl script that needs to be converted
Summary: Convert perl scripts to python → Convert preprocess-locale.pl to a python script
Attached patch patch in progress (obsolete) — Splinter Review
Tested with the ja and fa locales... this cleans things up quite a bit
Turns out that NSIS files that are not localized don't need to be converted to utf16le
Attachment #453333 - Attachment is obsolete: true
Attachment #453473 - Flags: review?(ted.mielczarek)
Attached patch 2. Thunderbird patch (obsolete) — Splinter Review
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.
Attachment #453509 - Flags: review?(philringnalda)
Attached patch 2. SeaMonkey patch (obsolete) — Splinter Review
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.
Attachment #453515 - Flags: review?(bugspam.Callek)
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.
Attached patch 2. SeaMonkey patch rev2 (obsolete) — Splinter Review
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.
Attachment #453626 - Flags: review?(bugspam.Callek)
Attached patch 2. Sunbird patch (obsolete) — Splinter Review
Simon, in case you want the Sunbird patch
Attachment #453628 - Flags: review?(bugzilla)
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
Attachment #453626 - Attachment is obsolete: true
Attachment #453626 - Flags: review?(bugspam.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 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).
I'm working on a fix so the requirement for iconv will be removed. The rest of the patch will remain the same.
Attachment #453515 - Flags: review?(bugspam.Callek) → review+
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
Attachment #453509 - Flags: review?(philringnalda) → review+
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.
Attachment #453628 - Flags: review?(bugzilla) → review+
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.
Attachment #453473 - Flags: feedback+
Attachment #453473 - Attachment is obsolete: true
Attachment #453473 - Flags: review?(ted.mielczarek)
Callek, thanks for the feedback... this addresses your comments
Attachment #453644 - Flags: review?(ted.mielczarek)
Attachment #453644 - Flags: feedback?(bugspam.Callek)
Attached patch 2. SeaMonkey patch rev2 (obsolete) — Splinter Review
This removes the requirement for iconv
Attachment #453515 - Attachment is obsolete: true
Attachment #453645 - Flags: review?(bugspam.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?
It doesn't if it is converted to utf-16-le by preprocess-locale.py
Attached patch 2. Sunbird patch (obsolete) — Splinter Review
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.
Attachment #453647 - Flags: review?(bugspam.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.
Attachment #453645 - Flags: review?(bugspam.Callek) → review+
Attached patch 2. comm-central configure patch (obsolete) — Splinter Review
Attachment #453649 - Flags: review?(bugspam.Callek)
Attachment #453649 - Flags: review?(bugspam.Callek) → review+
Attachment #453647 - Flags: review?(bugspam.Callek) → review+
Attachment #453628 - Attachment is obsolete: true
Attachment #453628 - Flags: review+
Attachment #453644 - Flags: feedback?(bugspam.Callek) → feedback+
Attachment #453644 - Flags: feedback+
Attachment #453509 - Flags: review+ → review?(philringnalda)
Attachment #453647 - Flags: review+ → review?(bugzilla)
Attachment #453649 - Attachment is obsolete: true
Attachment #453645 - Attachment is obsolete: true
Attachment #453645 - Flags: review+
Comment on attachment 453647 [details] [diff] [review]
2. Sunbird patch

Looks good.
r=sipaq
Attachment #453647 - Flags: review?(bugzilla) → review+
Attachment #453509 - Flags: review?(philringnalda) → review+
Ted ping
Sorry, Summit+Vacation back to back let my review queue get way behind.
Attachment #453505 - Flags: review?(ted.mielczarek) → review+
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.
Attachment #453644 - Flags: review?(ted.mielczarek) → review+
(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.
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.
(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.
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"))
Attachment #453644 - Attachment is obsolete: true
Attachment #460994 - Flags: review?(ted.mielczarek)
Comment on attachment 460994 [details] [diff] [review]
2. patch - convert preprocess-locale.pl to python rev3 (checked in)

Looks good, thanks!
Attachment #460994 - Flags: review?(ted.mielczarek) → review+
Attachment #460994 - Flags: approval2.0?
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 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.
Attachment #453505 - Flags: approval2.0?
Attachment #453505 - Flags: approval2.0? → approval2.0+
Attachment #460994 - Flags: approval2.0? → approval2.0+
Attachment #460994 - Attachment description: 1. patch - convert preprocess-locale.pl to python rev3 → 2. patch - convert preprocess-locale.pl to python rev3
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
Attachment #460994 - Attachment description: 2. patch - convert preprocess-locale.pl to python rev3 → 2. patch - convert preprocess-locale.pl to python rev3 (checked in)
No longer blocks: 571381
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.