Closed
Bug 1467897
Opened 7 years ago
Closed 7 years ago
Support local executions of build-clang
Categories
(Firefox Build System :: Toolchains, enhancement)
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: tjr, Assigned: tjr)
Details
Attachments
(1 file)
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 hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 3•7 years ago
|
||
(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 hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 8•7 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 10•7 years ago
|
||
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
Comment 11•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
Version: Version 3 → 3 Branch
You need to log in
before you can comment on or make changes to this bug.
Description
•