Closed
Bug 1284197
Opened 8 years ago
Closed 8 years ago
Make icu_sources_data.py runnable on Windows
Categories
(Core :: Internationalization, defect, P3)
Core
Internationalization
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: xidorn, Assigned: xidorn)
Details
Attachments
(1 file)
There are several issues for running icu_sources_data.py on Windows: 1. different path separator causes various issues 2. temporary files may not be deletable immediately after process terminates 3. windows doesn't support shebang Should be easy to fix.
Assignee | ||
Comment 1•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/62034/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/62034/
Attachment #8767583 -
Flags: review?(ted)
Assignee | ||
Comment 2•8 years ago
|
||
With this patch, icu_sources_data.py can run sucessfully on Windows, but a binary data file still gets changed :/
Comment 3•8 years ago
|
||
https://reviewboard.mozilla.org/r/62034/#review60474 ::: intl/icu_sources_data.py:22 (Diff revision 1) > +def to_unix_sep(filename): > + return filename.replace('\\', '/') > + > +def join_path(*args): > + return to_unix_sep(os.path.join(*args)) from mozpack import path as mozpath then use mozpath.normsep() and mozpath.join() instead of resp. to_unix_sep() and join_path(). (downside: you'll have to run mach python icu_sources_data.py, instead of python icu_sources_data.py) ::: intl/icu_sources_data.py:79 (Diff revision 1) > + try: > - os.unlink(f.name) > + os.unlink(f.name) > + except: > + print("Warning: failed to delete {}".format(f.name), > + file=sys.stderr) the os.unlink should be moved out of the with block instead of being ignored.
Assignee | ||
Comment 4•8 years ago
|
||
Comment on attachment 8767583 [details] Bug 1284197 - Make icu_sources_data.py runnable on Windows. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62034/diff/1-2/
Attachment #8767583 -
Flags: review?(ted) → review?(mh+mozilla)
Comment 5•8 years ago
|
||
Comment on attachment 8767583 [details] Bug 1284197 - Make icu_sources_data.py runnable on Windows. https://reviewboard.mozilla.org/r/62034/#review60484 ::: intl/icu_sources_data.py:64 (Diff revision 2) > for d in ['common', 'i18n']: > - makefile = os.path.join(topsrcdir, > + makefile = mozpath.join(topsrcdir, > 'intl/icu/source/%s/Makefile.in' % d) > - mozbuild = os.path.join(topsrcdir, > + mozbuild = mozpath.join(topsrcdir, > 'config/external/icu/%s/sources.mozbuild' % d) > - sources = [os.path.relpath(s, topsrcdir) > + sources = [mozpath.normsep(os.path.relpath(s, topsrcdir)) If you do mozpath.relpath, you get the mozpath.normsep for free. I should have mentioned that earlier. ::: intl/icu_sources_data.py:70 (Diff revision 2) > + succeeded = False > with tempfile.NamedTemporaryFile(prefix=name, delete=False) as f: > if subprocess.call(command, > stdout=f, > stderr=subprocess.STDOUT, > cwd=cwd, > **kwargs) == 0: > + succeeded = True > + if succeeded: enclose in a try/finish instead. ::: intl/icu_sources_data.py:140 (Diff revision 2) > + try: > - shutil.rmtree(objdir) > + shutil.rmtree(objdir) > + except: > + print("Warning: failed to remove {}".format(objdir), file=sys.stderr) Same as try_run, enclose with a try/finish instead of ignoring the error. Sorry I missed it the first time. ::: intl/icu_sources_data.py:153 (Diff revision 2) > if len(sys.argv) != 2: > print('Usage: icu_sources_data.py <mozilla topsrcdir>', > file=sys.stderr) > sys.exit(1) > > - topsrcdir = os.path.abspath(sys.argv[1]) > + topsrcdir = mozpath.normsep(os.path.abspath(sys.argv[1])) mozpath.abspath without mozpath.normsep :)
Attachment #8767583 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 6•8 years ago
|
||
Comment on attachment 8767583 [details] Bug 1284197 - Make icu_sources_data.py runnable on Windows. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62034/diff/2-3/
Attachment #8767583 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 7•8 years ago
|
||
I still cannot figure out why rmtree fails. There doesn't seem to be any overrun process which is using icu-config. I tried to run a tool from Windows Sysinternals called Handle [1] immediately before removing the directory to check whether there is any process accessing that file, but it showed nothing. [1] https://technet.microsoft.com/en-us/sysinternals/bb896655
Comment 8•8 years ago
|
||
Comment on attachment 8767583 [details] Bug 1284197 - Make icu_sources_data.py runnable on Windows. https://reviewboard.mozilla.org/r/62034/#review60550 ::: intl/icu_sources_data.py:20 (Diff revision 3) > import glob > import os > import shutil > import subprocess > import sys > +import time you're not using this ::: intl/icu_sources_data.py:75 (Diff revision 3) > def try_run(name, command, cwd=None, **kwargs): > + try: > - with tempfile.NamedTemporaryFile(prefix=name, delete=False) as f: > + with tempfile.NamedTemporaryFile(prefix=name, delete=False) as f: > - if subprocess.call(command, > - stdout=f, > - stderr=subprocess.STDOUT, > + subprocess.check_call(command, cwd=cwd, stdout=f, > + stderr=subprocess.STDOUT, **kwargs) > + except: except subprocess.CalledProcessError: ::: intl/icu_sources_data.py:140 (Diff revision 3) > - # Data file name has the major version number embedded. > + # Data file name has the major version number embedded. > - os.unlink(old_data_file) > + os.unlink(old_data_file) > - shutil.copy(new_data_file, tree_data_path) > + shutil.copy(new_data_file, tree_data_path) > - shutil.rmtree(objdir) > - return True > + return True > + finally: the try/finally pretty much defeats the whole "keep the log if it fails" in try_run, since rmtree will remote that log.
Attachment #8767583 -
Flags: review?(mh+mozilla)
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 8767583 [details] Bug 1284197 - Make icu_sources_data.py runnable on Windows. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62034/diff/3-4/
Attachment #8767583 -
Flags: review?(mh+mozilla)
Comment 10•8 years ago
|
||
Comment on attachment 8767583 [details] Bug 1284197 - Make icu_sources_data.py runnable on Windows. https://reviewboard.mozilla.org/r/62034/#review61418
Attachment #8767583 -
Flags: review?(mh+mozilla) → review+
Updated•8 years ago
|
Priority: -- → P3
Comment 11•8 years ago
|
||
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a85c5ad6051 Make icu_sources_data.py runnable on Windows. r=glandium
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9a85c5ad6051
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•