RFC: builders with implicit compile targets

[Paweł Hajdan, Jr.]

I'm working on https://code.google.com/p/chromium/issues/detail?id=536186 - a CQ false negative.

In my understanding, this is caused by builders like 'Android Arm64 Builder (dbg)' on chromium.linux using implicit compile targets - i.e. invoking ninja with no specific targets. This is fixed now with https://codereview.chromium.org/1371913005 .

There was one thing I could have handled better - the clang bots on FYI waterfall. I apologize for what was received as deliberately breaking the bots, which was not my intention. Please bear with me while I work on fixing it properly this time.

I'd like to understand why are clang FYI bots relying on implicit compile targets. Could you explain the context behind that?

Then, I'm looking for an alternative solution for https://code.google.com/p/chromium/issues/detail?id=536186 . While one might say all current trybots are fixed now, I'd like to have a check to prevent regressions.

In my opinion, Hans' fix https://codereview.chromium.org/1370213002 looks good. I wish I could understand the context behind that fix getting reverted by Nico, and what can we do better.

Pawel

[Dirk Pranke]

I suspect Nico may just not understand the full context behind the change or what you're trying to achieve.

He is correct that 'ninja all' is equivalent to 'ninja', and so the change at that level didn't have an effect, and so maybe he

thought it was a pointless CL.

On the other hand, maybe he did understand the context, but simply disagreed with it, or found some bug that I'm not 

seeing, either. Unfortunately, I think he's on vacation for a couple weeks and so may be quite slow to respond.

-- Dirk

--
You received this message because you are subscribed to the Google Groups "infra-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to infra-dev+...@chromium.org.
To post to this group, send email to infr...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/infra-dev/CAATLsPbQqamS64RFihz-yD22QfAN_h3zb6hiYhcGxumrtPOxiw%40mail.gmail.com.

[Nico Weber]

`ninja all` builds all targets, while just `ninja` builds the default target. The default target currently happens to be 'all' for all platforms, but that wasn't always the case (it used to be less than 'all' on the Android bots for a while, for example.)

The correct thing for bots that want to build everything is to invoke ninja without an explicit target. Maybe some branch in the recipes code thinks that "no explicit build targets" means "no build targets" instead of "default list of build targets" and early exits?

Somewhat in the same vein, I noticed that a compile bot failed to identify a compile problem I introduced, because it only compiled all test targets instead of the default target on a change that affected all code: http://crbug.com/544571 It feels like if analyze hits an exclusion, it should build the default target too (see that bug). That would probably resolve both these bugs?

[Dirk Pranke]

On Sat, Oct 17, 2015 at 10:12 PM, Nico Weber <tha...@chromium.org> wrote:

`ninja all` builds all targets, while just `ninja` builds the default target. The default target currently happens to be 'all' for all platforms, but that wasn't always the case (it used to be less than 'all' on the Android bots for a while, for example.)

The correct thing for bots that want to build everything is to invoke ninja without an explicit target. Maybe some branch in the recipes code thinks that "no explicit build targets" means "no build targets" instead of "default list of build targets" and early exits?

Why do you think running 'ninja' is preferable to 'ninja all'? Do you want to make sure we don't break the 'default' == 'all' logic, or do you think the extra four characters are redundant, or something else?

(My general inclination is that 'ninja all' is slightly clearer, but I don't feel super-strongly about this).

 

Somewhat in the same vein, I noticed that a compile bot failed to identify a compile problem I introduced, because it only compiled all test targets instead of the default target on a change that affected all code: http://crbug.com/544571 It feels like if analyze hits an exclusion, it should build the default target too (see that bug). That would probably resolve both these bugs?

I'm not sure I agree with suggestion in that bug. You could just as easily say that we should always build every target affected by a change, not just the test targets, but we don't do that now. Would you change that as well?

[Nico Weber]

On Mon, Oct 19, 2015 at 12:00 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Sat, Oct 17, 2015 at 10:12 PM, Nico Weber <tha...@chromium.org> wrote:

`ninja all` builds all targets, while just `ninja` builds the default target. The default target currently happens to be 'all' for all platforms, but that wasn't always the case (it used to be less than 'all' on the Android bots for a while, for example.)

The correct thing for bots that want to build everything is to invoke ninja without an explicit target. Maybe some branch in the recipes code thinks that "no explicit build targets" means "no build targets" instead of "default list of build targets" and early exits?

Why do you think running 'ninja' is preferable to 'ninja all'? Do you want to make sure we don't break the 'default' == 'all' logic, or do you think the extra four characters are redundant, or something else?

Yup, those two.

(Also, 'all' currently isn't all targets in gyp, but neither is the default target, so that's mostly an independent discussion: https://code.google.com/p/chromium/issues/detail?id=503241)

 

(My general inclination is that 'ninja all' is slightly clearer, but I don't feel super-strongly about this).

 

Somewhat in the same vein, I noticed that a compile bot failed to identify a compile problem I introduced, because it only compiled all test targets instead of the default target on a change that affected all code: http://crbug.com/544571 It feels like if analyze hits an exclusion, it should build the default target too (see that bug). That would probably resolve both these bugs?

I'm not sure I agree with suggestion in that bug. You could just as easily say that we should always build every target affected by a change, not just the test targets, but we don't do that now. Would you change that as well?

There should be some try bot that makes sure that all targets build fine. On the main waterfall, the clobber bots do this, but those don't do debug builds. So having the compile_dbg builds do this on the main waterfall would make sense to me.

[Dirk Pranke]

On Mon, Oct 19, 2015 at 12:14 PM, Nico Weber <tha...@chromium.org> wrote:

On Mon, Oct 19, 2015 at 12:00 PM, Dirk Pranke <dpr...@chromium.org> wrote:

On Sat, Oct 17, 2015 at 10:12 PM, Nico Weber <tha...@chromium.org> wrote:

`ninja all` builds all targets, while just `ninja` builds the default target. The default target currently happens to be 'all' for all platforms, but that wasn't always the case (it used to be less than 'all' on the Android bots for a while, for example.)

The correct thing for bots that want to build everything is to invoke ninja without an explicit target. Maybe some branch in the recipes code thinks that "no explicit build targets" means "no build targets" instead of "default list of build targets" and early exits?

Why do you think running 'ninja' is preferable to 'ninja all'? Do you want to make sure we don't break the 'default' == 'all' logic, or do you think the extra four characters are redundant, or something else?

Yup, those two.

(Also, 'all' currently isn't all targets in gyp, but neither is the default target, so that's mostly an independent discussion: https://code.google.com/p/chromium/issues/detail?id=503241)

Right.

 

(My general inclination is that 'ninja all' is slightly clearer, but I don't feel super-strongly about this).

 

Somewhat in the same vein, I noticed that a compile bot failed to identify a compile problem I introduced, because it only compiled all test targets instead of the default target on a change that affected all code: http://crbug.com/544571 It feels like if analyze hits an exclusion, it should build the default target too (see that bug). That would probably resolve both these bugs?

I'm not sure I agree with suggestion in that bug. You could just as easily say that we should always build every target affected by a change, not just the test targets, but we don't do that now. Would you change that as well?

There should be some try bot that makes sure that all targets build fine. On the main waterfall, the clobber bots do this, but those don't do debug builds. So having the compile_dbg builds do this on the main waterfall would make sense to me.

This sounds like several different suggestions getting bundled together:

1) making a change that affects all targets actually compile all targets, rather than just all requested targets

2) having some CQ bot actually build 'all'

3) having some waterfall debug bot actually build 'all'

4) (possibly?) having some CQ debug bot build 'all'?

We currently have the 'linux_chromium_clobber_rel_ng' bot running as a 100% experiment. It's doing a clobber build, but not building 'all'; I would have expected it to build 'all', but maybe I'm getting that confused w/ just doing a clobber.

jam@, what would you expect that bot to be doing?

See also the discussion in crbug.com/531296 ; we did have a clobber trybot that built all, but I removed it (somewhat unintentionally) a couple weeks back because I thought we had another bot doing this, but apparently we don't.

-- Dirk

[Paweł Hajdan, Jr.]

Thanks for commenting, Nico.

Based on your rationale, it's still unclear to me why you decided to revert https://codereview.chromium.org/1370213002 .

I can see "extra four characters that are redundant" as a legitimate preference, but I don't see it as severe enough to revert otherwise good CLs.

I was not aware of the second reason, i.e. to keep "default == all" behavior working. It seems a separate issue for me though (and is handled in a separate bug as mentioned above, https://code.google.com/p/chromium/issues/detail?id=503241). We can consider various solutions for that, possibly adding a separate recipe just for that case (arguably it doesn't fit well in existing chromium/chromium_tests design). I don't see any reason to conflate testing for that behavior with FYI clang bots.

I consider https://code.google.com/p/chromium/issues/detail?id=544571 related, but I'd recommend having another email thread about it if you'd like to. See https://groups.google.com/a/chromium.org/d/msg/infra-dev/zjn5f3-ZqKo/bH8c6XSRBQAJ for an attempt to sort it out.

My main goal is simplicity and ease of reasoning about the system. I've seen a lot of back-and-forth with how analyze should work (e.g. https://groups.google.com/a/chromium.org/d/msg/infra-dev/B-so7AN6-jE/inMkwNyEAAAJ), and I'm afraid we've been going in circles as a result of that confusion. In this thread I was exploring options to solve https://code.google.com/p/chromium/issues/detail?id=536186 which is another issue with analyze. My recommended solution is to require explicit compile targets for all users of chromium/chromium_tests infrastructure, since analyze's matching_exes requires the recipe to pass an explicit list of compile targets, for which it'll return the subset affected by the patch. Please let me know if you have alternative suggestions.

Paweł

[Nico Weber]

On Tue, Oct 20, 2015 at 5:40 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Thanks for commenting, Nico.

Based on your rationale, it's still unclear to me why you decided to revert https://codereview.chromium.org/1370213002 .

It didn't help, our bots were still red even with that change. So I wanted to revert https://codereview.chromium.org/1376453002/ (which added "Misconfigured bot: no compile targets" to almost 50 bot json files – I'm still baffled how this could land in the first place) via the web interface, and that was a prerequisite. (Reverting via web interface ended up not working for the second CL.)

 

I can see "extra four characters that are redundant" as a legitimate preference, but I don't see it as severe enough to revert otherwise good CLs.

I was not aware of the second reason, i.e. to keep "default == all" behavior working. It seems a separate issue for me though (and is handled in a separate bug as mentioned above, https://code.google.com/p/chromium/issues/detail?id=503241). We can consider various solutions for that, possibly adding a separate recipe just for that case (arguably it doesn't fit well in existing chromium/chromium_tests design). I don't see any reason to conflate testing for that behavior with FYI clang bots.

`ninja -C out/Release` is how the project has built all targets for years, so that's what these bots do.

[Nico Weber]

On Tue, Oct 20, 2015 at 7:32 AM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Oct 20, 2015 at 5:40 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Thanks for commenting, Nico.

Based on your rationale, it's still unclear to me why you decided to revert https://codereview.chromium.org/1370213002 .

It didn't help, our bots were still red even with that change. So I wanted to revert https://codereview.chromium.org/1376453002/ (which added "Misconfigured bot: no compile targets" to almost 50 bot json files – I'm still baffled how this could land in the first place) via the web interface, and that was a prerequisite. (Reverting via web interface ended up not working for the second CL.)

 

I can see "extra four characters that are redundant" as a legitimate preference, but I don't see it as severe enough to revert otherwise good CLs.

I was not aware of the second reason, i.e. to keep "default == all" behavior working. It seems a separate issue for me though (and is handled in a separate bug as mentioned above, https://code.google.com/p/chromium/issues/detail?id=503241). We can consider various solutions for that, possibly adding a separate recipe just for that case (arguably it doesn't fit well in existing chromium/chromium_tests design). I don't see any reason to conflate testing for that behavior with FYI clang bots.

`ninja -C out/Release` is how the project has built all targets for years, so that's what these bots do.

To clarify a bit here, making them build 'all' instead of '' is definitely a reasonable thing to discuss. (I'd argue against it for the reasons mentioned above, but I'd be willing to change my mind if there are good arguments why 'all' is better.) Breaking all our bots however and letting us deal with the fallout however isn't. Hans tried to fix, but as I said that fix didn't work, hence the revert. 

(See also https://code.google.com/p/chromium/issues/detail?id=446368 comments 6, 7, 8 for some context – I remember asking for advice on how to achieve this back then, and one of the things I tried was to get the bots to build 'all' instead of the default target. iannucci helped me a bit offline, but the people he told me were more knowledgeable about this area of recipes didn't reply to my questions. See CLs linked from those comments ;-) )

[Paweł Hajdan, Jr.]

I'm glad we're having this discussion. You brought up some facts I was not aware of. Let me comment on them.

1. I didn't know compiling "all" didn't work on the clang builders. Could you point me to an example failing build? What I could find is http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/2961 was the last build on that builder failing with "Misconfigured bot: no compile targets" error, while the next one - http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/2962 - compiled "all" and passed.

1a. There seems to have been an unrelated breakage very close in time, see e.g. http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/2965 . However, at that point the bot was no longer passing any targets to ninja, and the compile error was "ninja: error: '../../content/telemetry_gpu_unittests.isolate', needed by 'telemetry_gpu_unittests.isolated.gen.json', missing and no known rule to make it".

2. Looks like your comment about people not replying to your questions refers to me. Sorry about that. Unfortunately I'm behind on bugmail, and because of overload I also ignore comments on closed CLs, especially in cases where I'm not reviewer. I don't want to blame you - just wanted to say I'd happily help you say on infra-dev thread or on my @google.com email - or an IM ping.

Finally, I'd like to check - can we give it a try to make FYI clang builders build "all"? There's no way to test such a change before committing as far as I know, but I could do that e.g. early Europe working hours (~midnight PST) and watch the bots to minimize disruption. I'd obviously seek your sign-off on the CL before making any changes to your bots.

WDYT?

Paweł

[Nico Weber]

On Thu, Oct 22, 2015 at 5:04 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

I'm glad we're having this discussion. You brought up some facts I was not aware of. Let me comment on them.

1. I didn't know compiling "all" didn't work on the clang builders. Could you point me to an example failing build? What I could find is http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/2961 was the last build on that builder failing with "Misconfigured bot: no compile targets" error, while the next one - http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/2962 - compiled "all" and passed.

1a. There seems to have been an unrelated breakage very close in time, see e.g. http://build.chromium.org/p/chromium.fyi/builders/ClangToTLinux/builds/2965 . However, at that point the bot was no longer passing any targets to ninja, and the compile error was "ninja: error: '../../content/telemetry_gpu_unittests.isolate', needed by 'telemetry_gpu_unittests.isolated.gen.json', missing and no known rule to make it".

2. Looks like your comment about people not replying to your questions refers to me. Sorry about that. Unfortunately I'm behind on bugmail, and because of overload I also ignore comments on closed CLs, especially in cases where I'm not reviewer. I don't want to blame you - just wanted to say I'd happily help you say on infra-dev thread or on my @google.com email - or an IM ping.

Finally, I'd like to check - can we give it a try to make FYI clang builders build "all"? There's no way to test such a change before committing as far as I know, but I could do that e.g. early Europe working hours (~midnight PST) and watch the bots to minimize disruption. I'd obviously seek your sign-off on the CL before making any changes to your bots.

As I said, I don't consider bots that build the default target to be misconfigured. I'd strongly prefer if you could find a way to do what you want to do that didn't require breaking the way these bots have worked for years. 

[Paweł Hajdan, Jr.]

On Thu, Oct 22, 2015 at 6:49 PM, Nico Weber <tha...@chromium.org> wrote:

As I said, I don't consider bots that build the default target to be misconfigured.

Right, I'm sorry about the unfortunate wording in the error message. I'd like to focus on resolving the technical issue - https://code.google.com/p/chromium/issues/detail?id=536186 - and having explicit compile targets for all bots using the chromium recipe would really help for that.

 

I'd strongly prefer if you could find a way to do what you want to do that didn't require breaking the way these bots have worked for years. 

Hey Nico, I'm confused. In your previous message you said "making them build 'all' instead of '' is definitely a reasonable thing to discuss", which suggested you'd be more open to this than the above sentence indicates.

You've also mentioned builds broken by adding "all" to compile targets, but I couldn't find them. I offered to try the change at a time when you'd be offline so I can revert if it breaks without disrupting your workflow. Is there any reason that would not work for you?

While above would be an obvious reason not to compile "all" (i.e. if it fails), the other ones (saving few extra characters in an automatically run command or changing the way a bot worked for a long time) don't seem severe enough for what sounds like a strong opinion.

If you insist on passing no arguments to ninja, would it work for you if I converted the bots to their own recipe (e.g. chromium_clang)?

Paweł

[Paweł Hajdan, Jr.]

We talked offline, and I landed https://codereview.chromium.org/1447453002 .

Nico, I reviewed Clang builders on http://build.chromium.org/p/chromium.fyi/builders/ . They seem to either be OK, or failing before that change.

In case you see failures or I missed something, I don't mind at all if above change gets reverted - that'd be the right thing to do. Please just record what exactly failed so I could follow up.

If that works, as a next step I plan to re-introduce the previous change to fail compile if no targets are passed. I plan to let you review the change before it lands, make sure it doesn't break Clang bots when it lands, and similarly revert on first sign of trouble.

Looking forward to finally fixing this CQ issue in a way preventing future regressions, and thanks for bearing with me.

Paweł

[Nico Weber]

I usually look at http://build.chromium.org/p/chromium.fyi/console?category=win%20clang

http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/4278/steps/compile/logs/stdio seems to build both "all" and "crash_service" – do you know where "crash_service" comes from? I'm guessing that's from https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/recipe_modules/chromium_tests/api.py&l=329 which seems to be not in line with the "we don't want special cases in the recipes code" philosophy. But since that's already there, can we please not add crash_service as an explicit build target if either a) one of the build targets is already 'all' or b) add_tests_as_compile_targets is set?

[Dirk Pranke]

On Mon, Nov 16, 2015 at 10:44 AM, Nico Weber <tha...@chromium.org> wrote:

I usually look at http://build.chromium.org/p/chromium.fyi/console?category=win%20clang

http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/4278/steps/compile/logs/stdio seems to build both "all" and "crash_service" – do you know where "crash_service" comes from? I'm guessing that's from https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/recipe_modules/chromium_tests/api.py&l=329 which seems to be not in line with the "we don't want special cases in the recipes code" philosophy.

Correct, that's where it comes from. Unfortunately, there's no good other way at this time to get the behavior we want, 

which is to make sure that 'crash_service' is always built if we need to run tests.

So, yeah, we don't like special cases, but I haven't thought of a good way to not have one for this.

 

But since that's already there, can we please not add crash_service as an explicit build target if either a) one of the build targets is already 'all' or b) add_tests_as_compile_targets is set?

We could probably not add crash_service if 'all' is in compile_targets.

add_tests_as_compile_targets should probably go away; it would be better to have the bots that want that pass 'all' in 

as part of additional_compile_targets. Or, have a separate 'build_all' flag to be explicit (maybe that's the intent of

'add_tests_as_compile_targets': False, but if so it seems like 'build_all' would be a clearer name).

-- Dirk

[Nico Weber]

Pawel added an explicit "all" to the clang/win bots. Previously, this had the side effect of adding "all" to the build line but also so adding the 50 explicit targets added for tests, which makes it very easy to miss the "all" in there, and it also seems like bad UI to me.

What Pawel and I agreed on medium-term was that eventually we'd give a long list of targets to analyze and analyze then hands back what needs to be built, and since analyze knows about The Build, it would be in a good position to hand back just "all" if that's among the inputs. But until that's done, add_tests_as_compile_targets is what's supposed to prevent these very long build lines. I don't care much about how that flag is called, but I do care that the ninja invocation doesn't pass anything but "all" (or possibly even nothing, but "all" seems ok).

 

-- Dirk

[Dirk Pranke]

On Mon, Nov 16, 2015 at 11:14 AM, Nico Weber <tha...@chromium.org> wrote:

On Mon, Nov 16, 2015 at 11:05 AM, Dirk Pranke <dpr...@chromium.org> wrote:

On Mon, Nov 16, 2015 at 10:44 AM, Nico Weber <tha...@chromium.org> wrote:

I usually look at http://build.chromium.org/p/chromium.fyi/console?category=win%20clang

http://build.chromium.org/p/chromium.fyi/builders/CrWinClang/builds/4278/steps/compile/logs/stdio seems to build both "all" and "crash_service" – do you know where "crash_service" comes from? I'm guessing that's from https://code.google.com/p/chromium/codesearch#chromium/build/scripts/slave/recipe_modules/chromium_tests/api.py&l=329 which seems to be not in line with the "we don't want special cases in the recipes code" philosophy.

Correct, that's where it comes from. Unfortunately, there's no good other way at this time to get the behavior we want, 

which is to make sure that 'crash_service' is always built if we need to run tests.

So, yeah, we don't like special cases, but I haven't thought of a good way to not have one for this.

 

But since that's already there, can we please not add crash_service as an explicit build target if either a) one of the build targets is already 'all' or b) add_tests_as_compile_targets is set?

We could probably not add crash_service if 'all' is in compile_targets.

add_tests_as_compile_targets should probably go away; it would be better to have the bots that want that pass 'all' in 

as part of additional_compile_targets. Or, have a separate 'build_all' flag to be explicit (maybe that's the intent of

'add_tests_as_compile_targets': False, but if so it seems like 'build_all' would be a clearer name).

Pawel added an explicit "all" to the clang/win bots. Previously, this had the side effect of adding "all" to the build line but also so adding the 50 explicit targets added for tests, which makes it very easy to miss the "all" in there, and it also seems like bad UI to me.

Sure. Apart from the fact that 'all' doesn't actually mean all in GYP builds -- which I don't think we should worry about -- I think it's reasonable to

have the recipe be aware that if the list of compile_targets contains 'all' then passing either just ['all'] or even [] to compile.py is fine.

  

What Pawel and I agreed on medium-term was that eventually we'd give a long list of targets to analyze and analyze then hands back what needs to be built, and since analyze knows about The Build, it would be in a good position to hand back just "all" if that's among the inputs. But until that's done, add_tests_as_compile_targets is what's supposed to prevent these very long build lines.

In case this wasn't obvious, in the common case, analyze will not hand back 'all', because we prune to just the dependencies of all that are actually

affected by the patch (building all would cause us to build any targets that were out of date for other reasons).

If we hit a case analyze can't handle (like updates to build/common.gypi), 'all' would be returned, though.

I don't care much about how that flag is called, but I do care that the ninja invocation doesn't pass anything but "all" (or possibly even nothing, but "all" seems ok).

Understood.

-- Dirk

[Nico Weber]

Oh, even for non-trybots that just cycle all the time? Does that buy you anything over letting the build system do it in that case? I thought the point of analyze is that random other .o files might be out of date on the try jobs (because different tries sync to different revisions) and that analyze prevents building those. But on bots that just cycle all the time, that isn't true.

[Dirk Pranke]

We don't run analyze on those bots, for the reasons you say. Sorry if that wasn't clear.

-- Dirk

[Paweł Hajdan, Jr.]

Nico, please see https://codereview.chromium.org/1455563003 . Would that work for you?

Paweł

[Paweł Hajdan, Jr.]

Above change has been landed. Thanks for review.

As a next step, I uploaded https://codereview.chromium.org/1472893002 . I plan to monitor bots flagged by the expectations and add explicit compile targets as needed.

WDYT?

Paweł

[Nico Weber]

http://build.chromium.org/p/chromium.fyi/builders/ClangToTiOS/builds/4340/steps/compile/logs/stdio

doesn't seem to have any explicit compile target yet either, so that bot and probably all iOS bots (since that bot just uses the normal iOS recipe) will likely die when you reland this. So maybe check the iOS bots first.

Other than that, sounds good. Thanks for your patience!

[Paweł Hajdan, Jr.]

On Mon, Nov 23, 2015 at 4:43 PM, Nico Weber <tha...@chromium.org> wrote:

http://build.chromium.org/p/chromium.fyi/builders/ClangToTiOS/builds/4340/steps/compile/logs/stdio

doesn't seem to have any explicit compile target yet either, so that bot and probably all iOS bots (since that bot just uses the normal iOS recipe) will likely die when you reland this. So maybe check the iOS bots first.

That bot uses a different recipe, ios/unified_builder_tester .

Also, since src-side JSON files can only add compile targets, my reasoning is that if a bot is not flagged by test expectations for above CL, we can be sure it's not affected.

 

Other than that, sounds good. Thanks for your patience!

Cool.

Do you consider the comments on https://codereview.chromium.org/1472893002 blocking?

I agree we could make expectations more accurate, and I think it's ultimately blocked on moving recipes to chromium/src. Luke (luqui@) should know more details about that, consider asking him.

Paweł

[Nico Weber]

On Mon, Nov 23, 2015 at 11:27 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Mon, Nov 23, 2015 at 4:43 PM, Nico Weber <tha...@chromium.org> wrote:

http://build.chromium.org/p/chromium.fyi/builders/ClangToTiOS/builds/4340/steps/compile/logs/stdio

doesn't seem to have any explicit compile target yet either, so that bot and probably all iOS bots (since that bot just uses the normal iOS recipe) will likely die when you reland this. So maybe check the iOS bots first.

That bot uses a different recipe, ios/unified_builder_tester .

Also, since src-side JSON files can only add compile targets, my reasoning is that if a bot is not flagged by test expectations for above CL, we can be sure it's not affected.

 

Other than that, sounds good. Thanks for your patience!

Cool.

Do you consider the comments on https://codereview.chromium.org/1472893002 blocking?

Not having expectations files that say "This bot is broken!" for production bots seems like a good thing. If the recipes move will take a long time, maybe we could have a whitelist of bots with mock testing/buildbot entries in the recipes test code?

[Dirk Pranke]

On Mon, Nov 23, 2015 at 8:29 AM, Nico Weber <tha...@chromium.org> wrote:

On Mon, Nov 23, 2015 at 11:27 AM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

On Mon, Nov 23, 2015 at 4:43 PM, Nico Weber <tha...@chromium.org> wrote:

http://build.chromium.org/p/chromium.fyi/builders/ClangToTiOS/builds/4340/steps/compile/logs/stdio

doesn't seem to have any explicit compile target yet either, so that bot and probably all iOS bots (since that bot just uses the normal iOS recipe) will likely die when you reland this. So maybe check the iOS bots first.

That bot uses a different recipe, ios/unified_builder_tester .

Also, since src-side JSON files can only add compile targets, my reasoning is that if a bot is not flagged by test expectations for above CL, we can be sure it's not affected.

 

Other than that, sounds good. Thanks for your patience!

Cool.

Do you consider the comments on https://codereview.chromium.org/1472893002 blocking?

Not having expectations files that say "This bot is broken!" for production bots seems like a good thing. If the recipes move will take a long time, maybe we could have a whitelist of bots with mock testing/buildbot entries in the recipes test code?

As I just noted on the CL, I think the expectations are somewhere between misleading and wrong, so we should fix the test data. I'll work on a CL for that.

We have no ETA for the recipes move, so I wouldn't want to wait for that.

-- Dirk