Closed Bug 1284197 Opened 3 years ago Closed 3 years ago

Make icu_sources_data.py runnable on Windows

Categories

(Core :: Internationalization, defect, P3)

defect

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.
With this patch, icu_sources_data.py can run sucessfully on Windows, but a binary data file still gets changed :/
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.
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 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)
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)
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 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)
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 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+
Priority: -- → P3
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a85c5ad6051
Make icu_sources_data.py runnable on Windows. r=glandium
https://hg.mozilla.org/mozilla-central/rev/9a85c5ad6051
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.