1467897 - Support local executions of build-clang

Status: RESOLVED FIXED

(Firefox Build System :: Toolchains, enhancement)

Description

[Tom Ritter [:tjr]]

Currently, build-clang.py hardcodes a path that's used in our Docker workers. If we make that path configurable; then users who are patching the compiler can run build-clang.py locally (if they specify a json file that uses local paths.)

Comment 1

[Tom Ritter [:tjr]]

Attachment: Bug 1467897 Allow local runs of build-clang by providing a --base-dir option

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

Comment 2

[Mike Hommey [:glandium]]

Comment on attachment 8984577 [details]
Bug 1467897 Allow local runs of build-clang by providing a --base-dir option

https://reviewboard.mozilla.org/r/250450/#review256748

::: build/build-clang/build-clang.py:347
(Diff revision 1)
> +    if args.base_dir:
> +        base_dir = args.base_dir

This has no effect on windows because of the if is_windows() block below.

But actually, the is_windows() case seems about the right default for what to do locally, just ensure the directory doesn't exist first before doing anything that might overwrite existing files.

So, here's what I think would be nicer:
- when MOZ_AUTOMATION is set, default to '/builds/worker/....' on `not is_windows()`.
- otherwise, default to cwd + llvm-sources, or maybe change llvm-sources to build-clang or something.
- check that the directory that is picked does not exist.
- If it does, error out and tell the user to pass a base dir on the command line.

Comment 3

[Tom Ritter [:tjr]]

(In reply to Mike Hommey [:glandium] from comment #2)
> just ensure the directory doesn't exist first before doing
> anything that might overwrite existing files.
> 
> So, here's what I think would be nicer:
> - when MOZ_AUTOMATION is set, default to '/builds/worker/....' on `not
> is_windows()`.
> - otherwise, default to cwd + llvm-sources, or maybe change llvm-sources to
> build-clang or something.
> - check that the directory that is picked does not exist.
> - If it does, error out and tell the user to pass a base dir on the command
> line.

Hm. So we want to allow the user to work with existing directories; so they don't need to re-checkout the source trees everytime. So we can't just error if the existing directory exists at all.  I changed it around to drop a .build-clang and use that as an indicator if we can reuse the directory.

Comment 4

[Tom Ritter [:tjr]]

Comment on attachment 8984577 [details]
Bug 1467897 Allow local runs of build-clang by providing a --base-dir option

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

Comment 5

[Code Review Bot [:reviewbot]]

Comment on attachment 8984577 [details]
Bug 1467897 Allow local runs of build-clang by providing a --base-dir option

https://reviewboard.mozilla.org/r/250450/#review260710


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: build/build-clang/build-clang.py:374
(Diff revision 2)
> +        os.sys.exit(0)
> +
> +    if not os.path.exists(base_dir):
> +        os.makedirs(base_dir)
> +    elif os.listdir(base_dir) and '.build-clang' not in os.listdir(base_dir):
> +        raise ValueError("Base directory %s exists and is not a build-clang directory. " \

Error: The backslash is redundant between brackets [flake8: E502]

Comment 6

[Tom Ritter [:tjr]]

Comment on attachment 8984577 [details]
Bug 1467897 Allow local runs of build-clang by providing a --base-dir option

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

Comment 7

[Tom Ritter [:tjr]]

Comment on attachment 8984577 [details]
Bug 1467897 Allow local runs of build-clang by providing a --base-dir option

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

Comment 8

[Mike Hommey [:glandium]]

Comment on attachment 8984577 [details]
Bug 1467897 Allow local runs of build-clang by providing a --base-dir option

https://reviewboard.mozilla.org/r/250450/#review261516

::: build/build-clang/build-clang.py:333
(Diff revision 4)
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument('-c', '--config', required=True,
> +                        type=argparse.FileType('r'),
> +                        help="Clang configuration file")
> +    parser.add_argument('-b', '--base-dir', required=False,
> +                        type=str,

type=str shouldn't be necessary.

::: build/build-clang/build-clang.py:369
(Diff revision 4)
>      source_dir = base_dir + "/src"
>      build_dir = base_dir + "/build"
>  
> +    if not os.path.exists(base_dir):
> +        os.makedirs(base_dir)
> +    elif os.listdir(base_dir) and '.build-clang' not in os.listdir(base_dir):

os.path.exists(os.path.join(base_dir, '.build-clang'))

Comment 9

[Tom Ritter [:tjr]]

Comment on attachment 8984577 [details]
Bug 1467897 Allow local runs of build-clang by providing a --base-dir option

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250450/diff/4-5/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1cbc11323303
Allow local runs of build-clang by providing a --base-dir option r=glandium

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/1cbc11323303