Bug 184715 – Fix archive step for WinCairo build bot.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

Fix archive step for WinCairo build bot.

[Ross Kirsling]

Created attachment 338155 [details]
Patch

[Ross Kirsling]

*** Bug 149715 has been marked as a duplicate of this bug. ***

[Ross Kirsling]

Created attachment 338156 [details]
Patch

[Lucas Forschler]

Comment on attachment 338156 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338156&action=review

> Tools/BuildSlaveSupport/built-product-archive:216
> +        binType = 'bin64' if os.path.exists(os.path.join(_configurationBuildDirectory, 'bin64')) else 'bin32'

I'm working on a discussion with Windows folks here... if we have bots that build both 32bit and 64bit, it's possible this would always return bin64... breaking the 32bit case.
It might be best to put the wincairo logic into it's own else block to avoid any unexpected behavior on the 'win' platform.

[Brent Fulgham]

Comment on attachment 338156 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338156&action=review

>> Tools/BuildSlaveSupport/built-product-archive:216
>> +        binType = 'bin64' if os.path.exists(os.path.join(_configurationBuildDirectory, 'bin64')) else 'bin32'
> 
> I'm working on a discussion with Windows folks here... if we have bots that build both 32bit and 64bit, it's possible this would always return bin64... breaking the 32bit case.
> It might be best to put the wincairo logic into it's own else block to avoid any unexpected behavior on the 'win' platform.

I don't think we have any bots that work this way currently -- I think they build bin64 or bin32. We had talked about doing this for machines that only built occasionally, but never actually got around to doing so.

[Lucas Forschler]

(In reply to Brent Fulgham from comment #5)
> Comment on attachment 338156 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338156&action=review
> 
> >> Tools/BuildSlaveSupport/built-product-archive:216
> >> +        binType = 'bin64' if os.path.exists(os.path.join(_configurationBuildDirectory, 'bin64')) else 'bin32'
> > 
> > I'm working on a discussion with Windows folks here... if we have bots that build both 32bit and 64bit, it's possible this would always return bin64... breaking the 32bit case.
> > It might be best to put the wincairo logic into it's own else block to avoid any unexpected behavior on the 'win' platform.
> 
> I don't think we have any bots that work this way currently -- I think they
> build bin64 or bin32. We had talked about doing this for machines that only
> built occasionally, but never actually got around to doing so.

If that is the case, then this change as-is would not currently break anything... but would break something (32bit) if we do move to building both 64 and 32bit on the same machine in the future. My instinct thinks it would be much harder to debug a scenario like that in the future, and we should guard against it now. I do like the idea of using a flag... but we'd need to support that through the entire path (possibly including buildbot)

[Don Olmstead]

(In reply to Lucas Forschler from comment #6)
> (In reply to Brent Fulgham from comment #5)
> > Comment on attachment 338156 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=338156&action=review
> > 
> > >> Tools/BuildSlaveSupport/built-product-archive:216
> > >> +        binType = 'bin64' if os.path.exists(os.path.join(_configurationBuildDirectory, 'bin64')) else 'bin32'
> > > 
> > > I'm working on a discussion with Windows folks here... if we have bots that build both 32bit and 64bit, it's possible this would always return bin64... breaking the 32bit case.
> > > It might be best to put the wincairo logic into it's own else block to avoid any unexpected behavior on the 'win' platform.
> > 
> > I don't think we have any bots that work this way currently -- I think they
> > build bin64 or bin32. We had talked about doing this for machines that only
> > built occasionally, but never actually got around to doing so.
> 
> If that is the case, then this change as-is would not currently break
> anything... but would break something (32bit) if we do move to building both
> 64 and 32bit on the same machine in the future. My instinct thinks it would
> be much harder to debug a scenario like that in the future, and we should
> guard against it now. I do like the idea of using a flag... but we'd need to
> support that through the entire path (possibly including buildbot)

Is there any sort of logging or file naming that might clear this up? So that way for sure it shows its 32-bit or 64-bit in the case of debugging?

[Lucas Forschler]

It doesn't sound like there is any objection to landing this as-is... we just need to be careful in the future about any bot building both 32 and 64.

[WebKit Commit Bot]

Comment on attachment 338156 [details]
Patch

Clearing flags on attachment: 338156

Committed r230772: <https://trac.webkit.org/changeset/230772>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/39535892>