1284197 - Make icu_sources_data.py runnable on Windows

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P3)

Description

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1284197 - Make icu_sources_data.py runnable on Windows.

Review commit: https://reviewboard.mozilla.org/r/62034/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62034/

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

With this patch, icu_sources_data.py can run sucessfully on Windows, but a binary data file still gets changed :/

Comment 3

[Mike Hommey [:glandium] (high latency)]

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 4

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 5

[Mike Hommey [:glandium] (high latency)]

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 :)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

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

[Mike Hommey [:glandium] (high latency)]

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.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

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/

Comment 10

[Mike Hommey [:glandium] (high latency)]

Comment on attachment 8767583 [details]
Bug 1284197 - Make icu_sources_data.py runnable on Windows.

https://reviewboard.mozilla.org/r/62034/#review61418

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a85c5ad6051
Make icu_sources_data.py runnable on Windows. r=glandium

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9a85c5ad6051