983314 - Pass tooltool_script into misc.py, and make it work

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Michael Shal [:mshal]]

In bug 980120, I tried to pass the tooltool_script value from the config into misc.py. Unfortunately that breaks Linux builds because the tooltool_script path is '/builds/tooltool.py', which is the correct value to use inside of a mock environment, but outside of mock it should be '/tools/tooltool.py'

The fetch_and_unpack.sh script is executed outside of mock, so even though the configuration value uses the '/builds/...' path, it worked because the configuration was ignored here and instead relied on the default '/tools/...' path from buildbotcustom/process/factory.py. We can't just change the config to '/tools/...' because release.py also uses tooltool_script, and that executes tooltool.py inside mock.

I'd like to be able to pass in the configuration value so the tooltool.py path can be changed for Windows. To do so we need to reconcile the mis-match in Linux somehow. Here are a few possibilities we kicked around in IRC:

1) Make the mock path and the non-mock path of tooltool.py the same
  - This is a bit tricky because mock can't write to /tools, so they can't both be /tools/tooltool.py
  - bhearsum suggested maybe symlinking /builds/tooltool.py outside of mock

2) Run fetch_and_unpack.sh inside a mock environment, so it too can use /builds/tooltool.py

3) Add a new configuration variable so we can distinguish between tooltool_script inside and outside of mock.

I think 3) was pretty much universally disliked, but I'm open to arguments for/against the other two.

Comment 1

[Michael Shal [:mshal]]

Attachment: buildbotcustom-tooltool.patch

Here's what I tried for 2) run fetch_and_unpack.sh inside mock. It seemed to work, but I'm not sure what other side effects this might have.

Comment 2

[[e-mail] (:bhearsum)]

(In reply to Michael Shal [:mshal] from comment #0)
> 1) Make the mock path and the non-mock path of tooltool.py the same
>   - This is a bit tricky because mock can't write to /tools, so they can't
> both be /tools/tooltool.py
>   - bhearsum suggested maybe symlinking /builds/tooltool.py outside of mock

It looks like this should be easy to do. Just need to add a symlink into:
https://github.com/mozilla/build-puppet/blob/master/modules/packages/manifests/mozilla/tooltool.pp

(Example of symlinking with Puppet: https://mxr.mozilla.org/build-central/source/puppet-manifests/modules/buildslave/manifests/install.pp#209)

Comment 3

[Rail Aliiev [:rail]]

Comment on attachment 8390740 [details] [diff] [review]
buildbotcustom-tooltool.patch

lgtm. Feel free to remove "None"s (even from the lines above) - they are redundant.

Comment 4

[Michael Shal [:mshal]]

(In reply to Ben Hearsum [:bhearsum] from comment #2)
> It looks like this should be easy to do. Just need to add a symlink into:
> https://github.com/mozilla/build-puppet/blob/master/modules/packages/
> manifests/mozilla/tooltool.pp
> 
> (Example of symlinking with Puppet:
> https://mxr.mozilla.org/build-central/source/puppet-manifests/modules/
> buildslave/manifests/install.pp#209)

I just tried this out - it seems to be working fine too. Any preferences between the puppet change vs. running fetch_and_unpack.sh inside mock?

Comment 5

[Michael Shal [:mshal]]

Per IRC, we're going to try option 2 (symlink with puppet).

Comment 6

[Michael Shal [:mshal]]

Attachment: buildbotcustom-tooltool.patch

Pass the tooltool_script config variable in misc.py. I also removed the "None"s per rail's suggestion.

Note there is a tooltool_url_list referenced here: http://mxr.mozilla.org/build/source/buildbotcustom/misc.py#1375

It looks like it has something to do with mozharness - do we want/need to pass tooltool_script in there as well?

Comment 7

[Michael Shal [:mshal]]

Attachment: puppet.patch

Pupppet patch to add a symlink /builds/tooltool.py -> /tools/tooltool.py

Comment 8

[[e-mail] (:bhearsum)]

Comment on attachment 8391232 [details] [diff] [review]
buildbotcustom-tooltool.patch

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

Yup, this should be fine after the Puppet patch lands...

Comment 9

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8391233 [details] [diff] [review]
puppet.patch

I don't have any reason for an r- here, but it seems like this is just increasing the releng entropy.  Now there are two locations for tooltool?  Are they always both populated?  Do scripts search both locations?  Is it possible that one would be newer than the other?  Will we be mirroring /tools/* in /builds?

Comment 10

[[e-mail] (:bhearsum)]

(In reply to Dustin J. Mitchell [:dustin] from comment #9)
> Comment on attachment 8391233 [details] [diff] [review]
> puppet.patch
> 
> I don't have any reason for an r- here, but it seems like this is just
> increasing the releng entropy.  Now there are two locations for tooltool? 
> Are they always both populated?  Do scripts search both locations?  Is it
> possible that one would be newer than the other?  Will we be mirroring
> /tools/* in /builds?

This is a workaround for the fact that we can't put tooltool.py in /tools inside of mock. It's not really two locations, because it's a symlink, but it's not ideal. We could get back to one location in a couple of ways:
1) Always call tooltool.py from /builds
2) Make a tooltool RPM that installs it to /tools, so we can install it there in Mock.

I don't want to block Mike's work on either of those, though.

Comment 11

[Dustin J. Mitchell [:dustin] (he/him)]

I like the second option a lot.  Mike, can you land this and file a bug for Ben's second option?

Comment 12

[Michael Shal [:mshal]]

Comment on attachment 8391233 [details] [diff] [review]
puppet.patch

puppet change: https://hg.mozilla.org/build/puppet/rev/431000ef1bef

Comment 13

[Michael Shal [:mshal]]

Comment on attachment 8391232 [details] [diff] [review]
buildbotcustom-tooltool.patch

buildbotcustom: https://hg.mozilla.org/build/buildbotcustom/rev/a3bdcf6f2f16

Comment 14

[Rail Aliiev [:rail]]

in production

Comment 15

[Michael Shal [:mshal]]

Comment on attachment 8391232 [details] [diff] [review]
buildbotcustom-tooltool.patch

backed-out in https://hg.mozilla.org/build/buildbotcustom/rev/9c0ea7321650 because the default value of tooltool_script in process/factory.py no longer gets used.

Comment 16

[Michael Shal [:mshal]]

Attachment: buildbotcustom-tooltool.patch

The previous patch broke asan (and probably other) builds where tooltool_script wasn't set in the config. In these cases, the pf.get() would return None, which when passed into the factory kwargs would cause tooltool_script to be None instead of the default in the MercurialBuildFactory constructor. This new patch changes how the default in the factory is set.

Comment 17

[Michael Shal [:mshal]]

Comment on attachment 8393051 [details] [diff] [review]
buildbotcustom-tooltool.patch

buildbotcustom round 2: https://hg.mozilla.org/build/buildbotcustom/rev/0d0f05980bab

Comment 18

[Dustin J. Mitchell [:dustin] (he/him)]

Mike, did you file a bug for ben's suggestion in comment 10?

Comment 19

[Michael Shal [:mshal]]

(In reply to Dustin J. Mitchell [:dustin] from comment #18)
> Mike, did you file a bug for ben's suggestion in comment 10?

Yep, it's bug 983748.