Support local executions of build-clang

RESOLVED FIXED in Firefox 63

Status

Product:
Firefox Build System
Component:
Toolchains
Type:
enhancement
Status:
RESOLVED FIXED
Opened:
Last year
Updated:
7 months ago

People

(Reporter: tjr, Assigned: tjr)

Tracking

Version:
3 Branch
Target:
mozilla63

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(1 attachment)

Bug 1467897 Allow local runs of build-clang by providing a --base-dir option
59 bytes, text/x-review-board-request
glandium
: review+
Details 
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 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.
Attachment #8984577 - Flags: review?(mh+mozilla)
(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 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 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'))
Attachment #8984577 - Flags: review?(mh+mozilla) → review+
Keywords: checkin-needed
Pushed by rgurzau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1cbc11323303
Allow local runs of build-clang by providing a --base-dir option r=glandium
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1cbc11323303
Status: NEW → RESOLVED
Closed: Last year
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Version: Version 3 → 3 Branch
You need to log in before you can comment on or make changes to this bug.