1267454 - Move Java toolchain checks to Python configure

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Comment 1

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=212f36039c90

Comment 2

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1267454 - Add a parameter to find_program and check_prog to allow searching the given paths instead of $PATH.

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

Comment 3

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1267454 - Move java toolchain checks to Python configure.

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

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 8746716 [details]
MozReview Request: Bug 1267454 - Add a parameter to find_program and check_prog to allow searching the given paths instead of $PATH.

https://reviewboard.mozilla.org/r/49545/#review46883

::: build/moz.configure/checks.configure:96
(Diff revision 1)
>  # use that value instead.
>  @template
>  @imports(_from='mozbuild.shellutil', _import='quote')
>  @imports(_from='mozbuild.configure', _import='DependsFunction')
> -def check_prog(var, progs, what=None, input=None, allow_missing=False):
> +def check_prog(var, progs, what=None, input=None, allow_missing=False,
> +               path=None):

Might be better to name it "paths".

::: build/moz.configure/util.configure:36
(Diff revision 1)
>      return mozpath.normsep(path)
>  
>  
>  @imports(_from='which', _import='which')
>  @imports(_from='which', _import='WhichError')
> -def find_program(file):
> +def find_program(file, path=None):

same here, it would probably be better as paths, even if which takes it as "path".

::: build/moz.configure/util.configure:40
(Diff revision 1)
>  @imports(_from='which', _import='WhichError')
> -def find_program(file):
> +def find_program(file, path=None):
>      if is_absolute_or_relative(file):
>          return os.path.abspath(file) if os.path.isfile(file) else None
>      try:
> -        return normsep(which(file))
> +        return normsep(which(file, path=path))

type-check path?

And interesting question here is whether the argument should be considered additional paths or complete paths. That is, shouldn't we fallback to $PATH if the program is not found in the given paths?

::: python/mozbuild/mozbuild/test/configure/test_checks_configure.py:413
(Diff revision 1)
> +            ERROR: Cannot find a
> +        '''))
> +
> +        config, out, status = self.get_result('check_prog("A", ("known-a",), path=["/lib/other"])')
> +        self.assertEqual(status, 0)
> +        self.assertEqual(config, {'A': '/lib/other/known-a'})

Shouldn't that be self.OTHER_A here too? I'd be surprised if that works as is on Windows.

::: python/mozbuild/mozbuild/test/configure/test_checks_configure.py:421
(Diff revision 1)
> +        '''))
> +
> +        dirs = map(mozpath.dirname,
> +                   (self.OTHER_A, self.KNOWN_A, self.KNOWN_B, self.KNOWN_C))
> +        config, out, status = self.get_result(textwrap.dedent('''\
> +            check_prog("A", ("known-a",), path=["%s", "%s", "%s", "%s"])

I'm not sure this is testing something very different from the case above... You may instead want to check that /lib/other:/usr/bin finds /lib/other/known-a and that /usr/bin:/lib/other finds /usr/bin/known-a.

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8746717 [details]
MozReview Request: Bug 1267454 - Move java toolchain checks to Python configure.

https://reviewboard.mozilla.org/r/49547/#review46885

::: moz.configure:157
(Diff revision 1)
> +@depends('--with-java-bin-path')
> +@imports(_from='os', _import='environ')
> +def java_path(path):
> +    if path:
> +        # Look for javac and jar in the specified path.
> +        return [path[0]]

you /could/ just return path.

::: moz.configure:160
(Diff revision 1)
> +    if path:
> +        # Look for javac and jar in the specified path.
> +        return [path[0]]
> +    # With no path specified, look for javac and jar in $JAVA_HOME & $PATH.
> +    return ([os.path.join(environ.get('JAVA_HOME', ''), 'bin')] +
> +            environ.get('PATH', '').split(':'))

you want to split on os.pathsep here.

Note this sort of answers my question from the other patch. So we'd need both, and I somehow would feel better if we didn't have to split os.environ manually in callers. Adding another argument to find_program/check_prog? That might be a little heavy on arguments for check_prog... random thought: how about making it so that find_program replaces a "$PATH" item in paths with the split os.environ?

::: moz.configure:173
(Diff revision 1)
> +    def require_tool(result, require_java):
> +        if require_java and result is None:
> +            die('The program %s was not found.  Set \$JAVA_HOME to your Java '
> +                'SDK directory or use your Java SDK directory or use '
> +                '--with-java-bin-path={java-bin-dir}' % tool)

I know this is how things are today, but we should completely skip the test when java is not required. It could be as simple as putting this in e.g. build/moz.configure/java.configure and include that file from mobile/android/moz.configure.

Another way is to do something like
@template
def check_java_tool(tool):
    @depends(require_java)
    def candidates(require_java):
        if require_java:
            return (tool,)

    check = check_prog(tool.upper(), candidates, allow_missing=False)

Using allow_missing=False would make check_prog throw the error itself, and you wouldn't be able to have your own die, there. So you might still want allow_missing=True and the result check, until we have something better that would allow to kind of hijack the check_prog error (or add information to it).

::: moz.configure:191
(Diff revision 1)
> +javac = check_java_tool('javac')
> +
> +@depends(javac, require_java)
> +@checking('for minimum required javac version >= 1.7')
> +@imports('subprocess')
> +def java_version(javac, require_java):

javac_version

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8746716 [details]
MozReview Request: Bug 1267454 - Add a parameter to find_program and check_prog to allow searching the given paths instead of $PATH.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49545/diff/1-2/

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8746717 [details]
MozReview Request: Bug 1267454 - Move java toolchain checks to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49547/diff/1-2/

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8746716 [details]
MozReview Request: Bug 1267454 - Add a parameter to find_program and check_prog to allow searching the given paths instead of $PATH.

https://reviewboard.mozilla.org/r/49545/#review47469

::: build/moz.configure/util.configure:38
(Diff revision 2)
>  
>  @imports(_from='which', _import='which')
>  @imports(_from='which', _import='WhichError')
> -def find_program(file):
> +@imports('itertools')
> +@imports(_from='os', _import='pathsep')
> +def find_program(file, paths=None):

Would be worth documenting paths here too.

::: build/moz.configure/util.configure:43
(Diff revision 2)
> +            paths = list(itertools.chain(
> +                *(p.split(pathsep) for p in paths if p)))

You should type-check paths first, because if it happens to be a string, you'll end up with a list of all its characters.

Comment 9

[Mike Hommey [:glandium]]

Comment on attachment 8746717 [details]
MozReview Request: Bug 1267454 - Move java toolchain checks to Python configure.

https://reviewboard.mozilla.org/r/49547/#review47475

::: build/moz.configure/java.configure:20
(Diff revision 2)
> +def java_search_paths(path):
> +    if path:
> +        # Look for javac and jar in the specified path.
> +        return path
> +    # With no path specified, look for javac and jar in $JAVA_HOME & $PATH.
> +    return [os.path.join(environ.get('JAVA_HOME', ''), 'bin'),

If JAVA_HOME is not set, you get 'bin'. Not a nice path to search in. (I realize the code in old-configure does the same and ends up searching in /bin in that case... which is not better)

::: build/moz.configure/java.configure:21
(Diff revision 2)
> +    if path:
> +        # Look for javac and jar in the specified path.
> +        return path
> +    # With no path specified, look for javac and jar in $JAVA_HOME & $PATH.
> +    return [os.path.join(environ.get('JAVA_HOME', ''), 'bin'),
> +            environ.get('PATH', '')]

here, you can leave it to environ.get to return None, which will skip it in find_program.

::: build/moz.configure/java.configure:33
(Diff revision 2)
> +                       allow_missing=True)
> +
> +    @depends(check)
> +    def require_tool(result):
> +        if result is None:
> +            die('The program %s was not found.  Set \$JAVA_HOME to your Java '

You don't need a backslash for $.

::: build/moz.configure/java.configure:37
(Diff revision 2)
> +        if result is None:
> +            die('The program %s was not found.  Set \$JAVA_HOME to your Java '
> +                'SDK directory or use your Java SDK directory or use '
> +                '--with-java-bin-path={java-bin-dir}' % tool)
> +
> +    return check

Thinking about unit tests, it would be better to return require_tool and make require_tool return result when it doesn't die. BTW, it would be nice to have a unit test ;)

::: build/moz.configure/java.configure:47
(Diff revision 2)
> +check_java_tool('jarsigner')
> +check_java_tool('keytool')
> +javac = check_java_tool('javac')
> +
> +@depends(javac)
> +@checking('for minimum required javac version >= 1.7')

'for javac version'

::: build/moz.configure/java.configure:50
(Diff revision 2)
> +    output = subprocess.check_output([javac, '-version'],
> +                                     stderr=subprocess.STDOUT).rstrip()

It would be nicer to have an explicit die() if subprocess.CalledProcessError happens (like in yasm_version in toolchain.configure)

::: build/moz.configure/java.configure:54
(Diff revision 2)
> +@depends(javac_version)
> +def check_java_version(version):
> +    if version < '1.7':
> +        die('javac 1.7 or higher is required')

you can actually do this check in the javac_version function, and things should still be nice when it fails.

::: moz.configure
(Diff revision 2)
>              die('Cannot find Config.pm or $Config{archlib}. '
>                  'A full perl installation is required.')
>  
>  perl_version_check('5.006')
>  
> -

Please leave this line.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1267454 - Set up a mock-able import for os.environ in configure tests.

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

Comment 11

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1267454 - Allow passing kwargs to the mocked check_output in configure tests.

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

Comment 12

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8746716 [details]
MozReview Request: Bug 1267454 - Add a parameter to find_program and check_prog to allow searching the given paths instead of $PATH.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49545/diff/2-3/

Comment 13

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8746717 [details]
MozReview Request: Bug 1267454 - Move java toolchain checks to Python configure.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49547/diff/2-3/

Comment 14

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/49545/#review48351

::: build/moz.configure/util.configure:46
(Diff revisions 2 - 3)
>  def find_program(file, paths=None):
>      if is_absolute_or_relative(file):
>          return os.path.abspath(file) if os.path.isfile(file) else None
>      try:
>          if paths:
> +            if not isinstance(paths, list):

isinstance(paths, (list, tuple))

::: build/moz.configure/util.configure:48
(Diff revisions 2 - 3)
>          return os.path.abspath(file) if os.path.isfile(file) else None
>      try:
>          if paths:
> +            if not isinstance(paths, list):
> +                die("Paths provided to find_program must be a list of strings, "
> +                    "not %r" % paths)

You can do die(".... %r", paths)
(use a comma, not %)

Comment 15

[Mike Hommey [:glandium]]

Comment on attachment 8749458 [details]
MozReview Request: Bug 1267454 - Set up a mock-able import for os.environ in configure tests.

https://reviewboard.mozilla.org/r/50947/#review48353

::: python/mozbuild/mozbuild/test/configure/common.py:77
(Diff revision 1)
>          environ = dict(environ)
>          if 'CONFIG_SHELL' not in environ:
>              environ['CONFIG_SHELL'] = mozpath.abspath('/bin/sh')
>              self._subprocess_paths[environ['CONFIG_SHELL']] = self.shell
>              paths.append(environ['CONFIG_SHELL'])
> +        self._environ = environ

It might be better to duplicate the dict here. Or make it a ReadOnlyDict.

Comment 16

[Mike Hommey [:glandium]]

Comment on attachment 8749459 [details]
MozReview Request: Bug 1267454 - Allow passing kwargs to the mocked check_output in configure tests.

https://reviewboard.mozilla.org/r/50949/#review48357

Comment 17

[Mike Hommey [:glandium]]

Comment on attachment 8746717 [details]
MozReview Request: Bug 1267454 - Move java toolchain checks to Python configure.

https://reviewboard.mozilla.org/r/49547/#review48361

::: python/mozbuild/mozbuild/test/configure/test_checks_configure.py:544
(Diff revision 3)
> +        # We can use --with-java-bin-path instead of JAVA_HOME to similar
> +        # effect.

It would be worth adding a test for both JAVA_HOME and --with-java-bin-path being set to different things.

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8829e687a2b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/99e18039d973
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc3f12e88e51
https://hg.mozilla.org/integration/mozilla-inbound/rev/8934b6ad1630

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8829e687a2b8
https://hg.mozilla.org/mozilla-central/rev/99e18039d973
https://hg.mozilla.org/mozilla-central/rev/cc3f12e88e51
https://hg.mozilla.org/mozilla-central/rev/8934b6ad1630