1384706 - trychooser syntax should not invoke buildbot jobs by default

Status: RESOLVED FIXED

(Infrastructure & Operations Graveyard :: CIDuty, task)

Description

[Kim Moir [:kmoir] ET]

If you run -b do -p all  on try, the win buildbot build jobs run as well.  However, they shouldn't run now with the tcmigration of win32/win64

Comment 1

[Kim Moir [:kmoir] ET]

i.e. https://treeherder.mozilla.org/#/jobs?repo=try&revision=44e7c06da9dbf163c790a21b5679aab247a247bc

Comment 2

[Kim Moir [:kmoir] ET]

So would it be okay to change the scheduling for this to remove these extra windows builders on try but leave them on esr and release. Like what we did when removed them for mac. It seems like a cleaner change than changing the try syntax.

Comment 3

[Chris AtLee [:catlee]]

I think we wanted to leave them on try so that people had some way of testing patches for ESR/Release without having to land there.

At this point, I'm not sure how much value there is in that.

How hard is it to update the Try syntax?

Comment 4

[Kim Moir [:kmoir] ET]

I have some patches in progress to update the try syntax

Comment 5

[Kim Moir [:kmoir] ET]

Attachment: bug1384706.patch

These are my initial patches but looking at them, I realize I need to take a completely different approach.
I need to add flag like like --winlegacy instead of modifying another value like alllegacy to strip out win builds on all and when the platform is specified explicitly.  If this flag is enabled, I can allow the filter to pass windows builders, if not then remove them by default.  Might need to modify the taskcluster try parser too so it doesn't break with the new syntax as well.

Comment 6

[Kim Moir [:kmoir] ET]

Attachment: bug1384706-3.patch

Comment 7

[Kim Moir [:kmoir] ET]

Attachment: bug1384706-4.patch

I modified the bb side of the trychooser syntax builder to include a new value called --win or -w all to include windows platforms to be run.   Without this value, all the windows platforms will be removed from the list of builds that are run.  I also fixed existing tests, added new tests and commented out some tests that I don't think are relevant anymore.  I looked at the taskcluster trychooser code and I don't think it needs to be modified to accommodate this since all windows jobs should be triggered by tc and the syntax isn't the same anyways.

Comment 8

[Chris AtLee [:catlee]]

How about a '--buildbot' flag? If it's not set, then treat user_platforms as empty.

I think we need support for the old OSX jobs too, not just windows.

Comment 9

[Kim Moir [:kmoir] ET]

Attachment: bug1384706-5.patch

Comment 10

[Chris AtLee [:catlee]]

Comment on attachment 8899605 [details] [diff] [review]
bug1384706-5.patch

Review of attachment 8899605 [details] [diff] [review]:
-----------------------------------------------------------------

::: try_parser.py
@@ +312,5 @@
>                          default='none',
>                          dest='talos',
>                          help='provide a list of talos tests, or specify all (default is None)')
> +    parser.add_argument('--buildbot',
> +                        default='none',

Should this be None? Or turn this into a boolean switch - see comments below.

@@ +397,2 @@
>          else:
>              user_platforms.add(platform)

If my try syntax includes '-p win32', then we would get buildbot jobs due to this line too.

What about if '--buildbot' isn't present, then user_platforms is just set(), and we exit?

So the try syntax required for windows buildbot jobs would be something like "try: -b do -p win32 --buildbot"

Comment 11

[Kim Moir [:kmoir] ET]

Attachment: bug1384706-6.patch

Comment 12

[Kim Moir [:kmoir] ET]

Attachment: bug1384706-8.patch

Comment 13

[Chris AtLee [:catlee]]

Comment on attachment 8902401 [details] [diff] [review]
bug1384706-8.patch

Review of attachment 8902401 [details] [diff] [review]:
-----------------------------------------------------------------

looks great, thanks!

Comment 15

[Kim Moir [:kmoir] ET]

Attachment: bug1384706-9.patch

patch with commented out tests removed.

Comment 16

[Kim Moir [:kmoir] ET]

Attachment: github pr

Comment 17

[Kim Moir [:kmoir] ET]

verified that the spurious windows jobs aren't running on try, still have to deploy a new version of releng services in bug 1397326 to fix the try chooser syntax

Comment 18

[Aki Sasaki (not active)]

I think this broke try-comm-central.

Comment 19

[Tom Prince [:tomprince]]

This did break try-comm-central specifying linux/linux64 builds (see Bug 1402808).

Comment 20

[Kim Moir [:kmoir] ET]

The trychooser deploy happened today so closing.