Closed
Bug 1284197
Opened 9 years ago
Closed 9 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•9 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•9 years ago
|
||
With this patch, icu_sources_data.py can run sucessfully on Windows, but a binary data file still gets changed :/
Comment 3•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 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•9 years ago
|
Priority: -- → P3
Comment 11•9 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•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 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
•