Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2038483002:
mb: Allow overriding the path to the gyp script (Closed)
|
Created:
4 years, 6 months ago by Michael Achenbach Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionmb: Allow overriding the path to the gyp script
This allows changing the path to make this script usable
in other projects than chromium.
BUG=616035
Committed: https://crrev.com/60ebf6206b4a14e6b4be903fb42bd34d6b287645
Cr-Commit-Position: refs/heads/master@{#398296}
Patch Set 1 #Patch Set 2 : Docu #Patch Set 3 : Review #
Total comments: 5
Patch Set 4 : Review #Messages
Total messages: 32 (13 generated)
Description was changed from ========== mb: Allow owerriding the path to the gyp script BUG= ========== to ========== mb: Allow owerriding the path to the gyp script BUG=616035 ==========
Description was changed from ========== mb: Allow owerriding the path to the gyp script BUG=616035 ========== to ========== mb: Allow overriding the path to the gyp script This allows changing the path to make this script usable in other projects than chromium. BUG=616035 ==========
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/patch-status/2038483002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2038483002/20001
machenbach@chromium.org changed reviewers: + dpranke@chromium.org, jochen@chromium.org
PTAL. This is required to use mb in v8. WIP CL: https://codereview.chromium.org/2024893002/
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The path to GYP isn't something that is set inside build files or used to generate ninja itself, so it doesn't really seem to me to be the sort of thing that should be in a mixin. Can you just make this a command line arg instead?
On 2016/06/02 16:18:04, Dirk Pranke wrote: > The path to GYP isn't something that is set inside build files or used to > generate ninja itself, so it doesn't really seem to me to be the sort of thing > that should be in a mixin. > > Can you just make this a command line arg instead? I would also be fine with a change that changed the default logic for how MB searches for a config file to something that would work automatically if we were in a v8 repo. If MB is going to be used across multiple products it makes sense to move the default location for the config file someplace other than //tools/mb, I think, but I don't want to spend too much time on this at the moment since as I've noted elsewhere MB should ultimately be replaced with something else in a post-GYP world.
On 2016/06/02 17:43:17, Dirk Pranke wrote: > On 2016/06/02 16:18:04, Dirk Pranke wrote: > > The path to GYP isn't something that is set inside build files or used to > > generate ninja itself, so it doesn't really seem to me to be the sort of thing > > that should be in a mixin. > > > > Can you just make this a command line arg instead? Can do. I'll also need to add this cmd line arg at the call site in the chromium recipe https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/... as I'd like to reuse that method. > > I would also be fine with a change that changed the default logic for how MB > searches > for a config file to something that would work automatically if we were in a v8 > repo. I could move the config to some place that is not depsed in any repo. But to get things done, I think it'd be much easier to just pass a custom v8 mb_config_path here: https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/...
On 2016/06/02 18:15:22, Michael Achenbach wrote: > On 2016/06/02 17:43:17, Dirk Pranke wrote: > > On 2016/06/02 16:18:04, Dirk Pranke wrote: > > > The path to GYP isn't something that is set inside build files or used to > > > generate ninja itself, so it doesn't really seem to me to be the sort of > thing > > > that should be in a mixin. > > > > > > Can you just make this a command line arg instead? > > Can do. I'll also need to add this cmd line arg at the call site in the chromium > recipe > https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/... > as I'd like to reuse that method. Yup, that's fine. > > > > I would also be fine with a change that changed the default logic for how MB > > searches > > for a config file to something that would work automatically if we were in a > v8 > > repo. > > I could move the config to some place that is not depsed in any repo. But to get > things done, I think it'd be much easier to just pass a custom v8 mb_config_path > here: > https://cs.chromium.org/chromium/build/scripts/slave/recipe_modules/chromium/... Yes, that's definitely easier and if there's no real need to have this work out of the box for devs, it's the way to go (it's what we do in other non-chromium repos today, but you're the first one who has needed to change the path to gyp).
Done. PTAL https://codereview.chromium.org/2038483002/diff/40001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2038483002/diff/40001/tools/mb/mb.py#newcode87 tools/mb/mb.py:87: subp.add_argument('--gyp-script', FYI: I didn't specify a default here to keep the tests happy. PathJoin is not mocked yet when this is called.
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/patch-status/2038483002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/2038483002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ nits. https://codereview.chromium.org/2038483002/diff/40001/tools/mb/docs/user_guid... File tools/mb/docs/user_guide.md (right): https://codereview.chromium.org/2038483002/diff/40001/tools/mb/docs/user_guid... tools/mb/docs/user_guide.md:138: The gyp script defaults to `build/gyp_chromium`, but can be overridden with Nit: I would have written "//build/gyp_chromium" to be a bit clearer. https://codereview.chromium.org/2038483002/diff/40001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2038483002/diff/40001/tools/mb/mb.py#newcode87 tools/mb/mb.py:87: subp.add_argument('--gyp-script', On 2016/06/03 07:36:38, Michael Achenbach wrote: > FYI: I didn't specify a default here to keep the tests happy. PathJoin is not > mocked yet when this is called. I don't quite understand this comment; PathJoin() should already be defined on the object (both here and in the subclass)? However, alternatively, you could define a .default_gyp_script in the __init__() method just like there's a default_config. It's not all that important either way, though.
https://codereview.chromium.org/2038483002/diff/40001/tools/mb/docs/user_guid... File tools/mb/docs/user_guide.md (right): https://codereview.chromium.org/2038483002/diff/40001/tools/mb/docs/user_guid... tools/mb/docs/user_guide.md:138: The gyp script defaults to `build/gyp_chromium`, but can be overridden with On 2016/06/03 17:59:24, Dirk Pranke wrote: > Nit: I would have written "//build/gyp_chromium" to be a bit clearer. Can do. But is it clearer? Doesn't a // suggest that this is the syntax that should be used? Passing --gyp-script=//build/gyp_chromium wouldn't work as the logic expects a relative path. WDYT? I can also just make two sentences. One more with an example about what to pass in. https://codereview.chromium.org/2038483002/diff/40001/tools/mb/mb.py File tools/mb/mb.py (right): https://codereview.chromium.org/2038483002/diff/40001/tools/mb/mb.py#newcode87 tools/mb/mb.py:87: subp.add_argument('--gyp-script', On 2016/06/03 17:59:24, Dirk Pranke wrote: > On 2016/06/03 07:36:38, Michael Achenbach wrote: > > FYI: I didn't specify a default here to keep the tests happy. PathJoin is not > > mocked yet when this is called. > > I don't quite understand this comment; PathJoin() should already be defined on > the object (both here and in the subclass)? > > However, alternatively, you could define a .default_gyp_script in the __init__() > method just like there's a default_config. > > It's not all that important either way, though. That's what I tried. PathJoin was not defined in the init method. And setting default_gyp_script with path.join there made the test fail on windows. Probably it's defined in ParseArgs. I could just call it there to calculate the default on the fly without setting it in a field.
PTAL at patch 4.
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/patch-status/2038483002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by machenbach@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org, dpranke@chromium.org Link to the patchset: https://codereview.chromium.org/2038483002/#ps60001 (title: "Review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2038483002/60001
Message was sent while issue was closed.
Description was changed from ========== mb: Allow overriding the path to the gyp script This allows changing the path to make this script usable in other projects than chromium. BUG=616035 ========== to ========== mb: Allow overriding the path to the gyp script This allows changing the path to make this script usable in other projects than chromium. BUG=616035 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== mb: Allow overriding the path to the gyp script This allows changing the path to make this script usable in other projects than chromium. BUG=616035 ========== to ========== mb: Allow overriding the path to the gyp script This allows changing the path to make this script usable in other projects than chromium. BUG=616035 Committed: https://crrev.com/60ebf6206b4a14e6b4be903fb42bd34d6b287645 Cr-Commit-Position: refs/heads/master@{#398296} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/60ebf6206b4a14e6b4be903fb42bd34d6b287645 Cr-Commit-Position: refs/heads/master@{#398296} |
