Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2106423002:
[gn] Automatically derive build configs in test runner. (Closed)
|
Created:
4 years, 5 months ago by Michael Achenbach Modified:
4 years, 5 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[gn] Automatically derive build configs in test runner.
This executes an action as part of the build, writing a json
configuration that includes all build flags relevant to v8
testing.
The test runner will derive all build-dependent flags from
the file if it detects it.
BUG=chromium:474921
Committed: https://crrev.com/f4dd32319dcc61f3bbf33e17c8d4fa65cb9a39ab
Cr-Commit-Position: refs/heads/master@{#37446}
Patch Set 1 #Patch Set 2 : [gn] Automatically derive build configs in test runner. #
Total comments: 9
Patch Set 3 : Review #Patch Set 4 : Presubmit + more robust json loading #
Dependent Patchsets: Messages
Total messages: 21 (10 generated)
Description was changed from ========== [gn] Automatically derive build configs in test runner. BUG= ========== to ========== [gn] Automatically derive build configs in test runner. This executes an action as part of the build, writing a json configuration that includes all build flags relevant to v8 testing. The test runner will derive all build-dependent flags from the file if it detects it. BUG=chromium:474921 ==========
machenbach@chromium.org changed reviewers: + jkummerow@chromium.org, jochen@chromium.org, vogelheim@chromium.org
PTAL
Looks mostly good, but IMHO the error value thing needs to be fixed, since build steps with silent failures are teh cancer. The use-gn-args-directly thing would be a bigger change, and I'm not entirely sure it's even the right idea, so.. please consider it, but proceed in whichever way you prefer. https://codereview.chromium.org/2106423002/diff/20001/tools/run-tests.py File tools/run-tests.py (right): https://codereview.chromium.org/2106423002/diff/20001/tools/run-tests.py#newc... tools/run-tests.py:439: # used. This can't be overridden by cmd-line arguments. Clever. I really like this! I still have something to [complain|improve], though: :) Would it make sense to - instead of checking for v8_build_config.json, to check for presence of args.gn, and if so use the result of "gn args out/XXX --list --short" and extract the value from there? It seems like every single variable we use below should be in there. Also, every future variable declared in gn would be, too. And we don't need an additional script and build step. https://codereview.chromium.org/2106423002/diff/20001/tools/testrunner/utils/... File tools/testrunner/utils/dump_build_config.py (right): https://codereview.chromium.org/2106423002/diff/20001/tools/testrunner/utils/... tools/testrunner/utils/dump_build_config.py:27: with open(os.path.join(sys.argv[1], 'v8_build_config.json'), 'w') as f: os.path.join(...): This is not what the header comment says. Why not make sys.argv[1] the actual full file name, rather than a path w/ an implied name? https://codereview.chromium.org/2106423002/diff/20001/tools/testrunner/utils/... tools/testrunner/utils/dump_build_config.py:28: json.dump(dict(as_json(kv) for kv in sys.argv[2:]), f) please add a try-except for IOError, and return -1 (or however that works in python.) (Because: Since this is meant to run as part of the build, we should make sure it will return non-zero value in case of errors.)
Changes and replies. PTAL https://codereview.chromium.org/2106423002/diff/20001/tools/run-tests.py File tools/run-tests.py (right): https://codereview.chromium.org/2106423002/diff/20001/tools/run-tests.py#newc... tools/run-tests.py:439: # used. This can't be overridden by cmd-line arguments. On 2016/06/30 11:08:02, vogelheim wrote: > Clever. I really like this! > > I still have something to [complain|improve], though: :) > > Would it make sense to - instead of checking for v8_build_config.json, to check > for presence of args.gn, and if so use the result of "gn args out/XXX --list > --short" and extract the value from there? > > It seems like every single variable we use below should be in there. Also, every > future variable declared in gn would be, too. And we don't need an additional > script and build step. > Would be nice, unfortunately, the --list command shows you only the default values. Not what is actually set. And I didn't wanna write a parser for the gn.args file. https://codereview.chromium.org/2106423002/diff/20001/tools/testrunner/utils/... File tools/testrunner/utils/dump_build_config.py (right): https://codereview.chromium.org/2106423002/diff/20001/tools/testrunner/utils/... tools/testrunner/utils/dump_build_config.py:27: with open(os.path.join(sys.argv[1], 'v8_build_config.json'), 'w') as f: On 2016/06/30 11:08:02, vogelheim wrote: > os.path.join(...): This is not what the header comment says. Why not make > sys.argv[1] the actual full file name, rather than a path w/ an implied name? Done. But as a consequence I needed to remove the assertions. I can't call dirname on sys.argv[1] because the script is executed in the outdir and there are no slashes passed by gn. Just the name of the json file. Before, gn passed the weird construction ../Release if Release was the out dir. https://codereview.chromium.org/2106423002/diff/20001/tools/testrunner/utils/... tools/testrunner/utils/dump_build_config.py:28: json.dump(dict(as_json(kv) for kv in sys.argv[2:]), f) On 2016/06/30 11:08:02, vogelheim wrote: > please add a try-except for IOError, and return -1 (or however that works in > python.) > > (Because: Since this is meant to run as part of the build, we should make sure > it will return non-zero value in case of errors.) Not sure what you mean. When an exception is thrown to top-level, the return code will be -1. I just tried to add some wrong json in the build file and then this step fails nicely. Or do you want to avoid seeing the stack trace of the exception?
lgtm https://codereview.chromium.org/2106423002/diff/20001/tools/run-tests.py File tools/run-tests.py (right): https://codereview.chromium.org/2106423002/diff/20001/tools/run-tests.py#newc... tools/run-tests.py:439: # used. This can't be overridden by cmd-line arguments. On 2016/06/30 11:47:49, Michael Achenbach wrote: > On 2016/06/30 11:08:02, vogelheim wrote: > > Clever. I really like this! > > > > I still have something to [complain|improve], though: :) > > > > Would it make sense to - instead of checking for v8_build_config.json, to > check > > for presence of args.gn, and if so use the result of "gn args out/XXX --list > > --short" and extract the value from there? > > > > It seems like every single variable we use below should be in there. Also, > every > > future variable declared in gn would be, too. And we don't need an additional > > script and build step. > > > > Would be nice, unfortunately, the --list command shows you only the default > values. Not what is actually set. And I didn't wanna write a parser for the > gn.args file. Pity. I had indeed misunderstood what --list does. :-( https://codereview.chromium.org/2106423002/diff/20001/tools/testrunner/utils/... File tools/testrunner/utils/dump_build_config.py (right): https://codereview.chromium.org/2106423002/diff/20001/tools/testrunner/utils/... tools/testrunner/utils/dump_build_config.py:27: with open(os.path.join(sys.argv[1], 'v8_build_config.json'), 'w') as f: On 2016/06/30 11:47:50, Michael Achenbach wrote: > On 2016/06/30 11:08:02, vogelheim wrote: > > os.path.join(...): This is not what the header comment says. Why not make > > sys.argv[1] the actual full file name, rather than a path w/ an implied name? > > Done. But as a consequence I needed to remove the assertions. I can't call > dirname on sys.argv[1] because the script is executed in the outdir and there > are no slashes passed by gn. Just the name of the json file. Before, gn passed > the weird construction ../Release if Release was the out dir. I guess you could use os.path.absname to first construct the absolute path, but I don't think there's much value in that... https://codereview.chromium.org/2106423002/diff/20001/tools/testrunner/utils/... tools/testrunner/utils/dump_build_config.py:28: json.dump(dict(as_json(kv) for kv in sys.argv[2:]), f) On 2016/06/30 11:47:49, Michael Achenbach wrote: > On 2016/06/30 11:08:02, vogelheim wrote: > > please add a try-except for IOError, and return -1 (or however that works in > > python.) > > > > (Because: Since this is meant to run as part of the build, we should make sure > > it will return non-zero value in case of errors.) > > Not sure what you mean. When an exception is thrown to top-level, the return > code will be -1. I just tried to add some wrong json in the build file and then > this step fails nicely. Ah, that is what I meant. I had assumed an uncaught exception to do something different. :)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/18545)
Patch 4 is more robust to invalid json. If e.g. a ninja build is started and then canceled, the output file is still there, but empty.
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by machenbach@chromium.org
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vogelheim@chromium.org Link to the patchset: https://codereview.chromium.org/2106423002/#ps60001 (title: "Presubmit + more robust json loading")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [gn] Automatically derive build configs in test runner. This executes an action as part of the build, writing a json configuration that includes all build flags relevant to v8 testing. The test runner will derive all build-dependent flags from the file if it detects it. BUG=chromium:474921 ========== to ========== [gn] Automatically derive build configs in test runner. This executes an action as part of the build, writing a json configuration that includes all build flags relevant to v8 testing. The test runner will derive all build-dependent flags from the file if it detects it. BUG=chromium:474921 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== [gn] Automatically derive build configs in test runner. This executes an action as part of the build, writing a json configuration that includes all build flags relevant to v8 testing. The test runner will derive all build-dependent flags from the file if it detects it. BUG=chromium:474921 ========== to ========== [gn] Automatically derive build configs in test runner. This executes an action as part of the build, writing a json configuration that includes all build flags relevant to v8 testing. The test runner will derive all build-dependent flags from the file if it detects it. BUG=chromium:474921 Committed: https://crrev.com/f4dd32319dcc61f3bbf33e17c8d4fa65cb9a39ab Cr-Commit-Position: refs/heads/master@{#37446} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/f4dd32319dcc61f3bbf33e17c8d4fa65cb9a39ab Cr-Commit-Position: refs/heads/master@{#37446} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
