Bug 215419 – [webkitpy] Use webkitcorepy's auto installer for coverage

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

Part of removing webkitpy's autoinstaller in favor of webkitcorepy's.

[Radar WebKit Bug Importer]

<rdar://problem/66922400>

[Jonathan Bedard]

Created attachment 406467 [details]
Patch

[Jonathan Bedard]

Created attachment 406472 [details]
Patch

[Jonathan Bedard]

Created attachment 406474 [details]
Patch

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #4)
> Created attachment 406474 [details]
> Patch

Didn't realize we had toml in the autoinstall list, then didn't realize that flatpack was using it directly.

[Stephanie Lewis]

Comment on attachment 406474 [details]
Patch

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

> Tools/Scripts/webkitpy/__init__.py:31
> +AutoInstall.register(Package('toml', Version(0, 10, 1)))

Still don't like not finding dependencies automatically.  Going to add a lot of extraneous code.  Why are we not using requires files?

[Jonathan Bedard]

(In reply to Stephanie Lewis from comment #6)
> Comment on attachment 406474 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406474&action=review
> 
> > Tools/Scripts/webkitpy/__init__.py:31
> > +AutoInstall.register(Package('toml', Version(0, 10, 1)))
> 
> Still don't like not finding dependencies automatically.  Going to add a lot
> of extraneous code.  Why are we not using requires files?

The biggest issue with parsing requires files is that older bots will have different library versions installed than newer ones.

This isn't totally theoretical. We had an issue a few months back with certifi (a dependency of requests), where newer bots didn't have the issue, but older ones did (because they had an older version of certifi). The solution to this is to version lock the dependencies, which leaves us back at listing out dependencies and their versions. We could pick and choose the dependencies we version lock, but the issue with this is that we won't know which dependencies we need to version lock until we encounter a bug, and I feel like we'd spend more time investigating such bugs then just version locking everything.

[Stephanie Lewis]

Comment on attachment 406474 [details]
Patch

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

>>> Tools/Scripts/webkitpy/__init__.py:31
>>> +AutoInstall.register(Package('toml', Version(0, 10, 1)))
>> 
>> Still don't like not finding dependencies automatically.  Going to add a lot of extraneous code.  Why are we not using requires files?
> 
> The biggest issue with parsing requires files is that older bots will have different library versions installed than newer ones.
> 
> This isn't totally theoretical. We had an issue a few months back with certifi (a dependency of requests), where newer bots didn't have the issue, but older ones did (because they had an older version of certifi). The solution to this is to version lock the dependencies, which leaves us back at listing out dependencies and their versions. We could pick and choose the dependencies we version lock, but the issue with this is that we won't know which dependencies we need to version lock until we encounter a bug, and I feel like we'd spend more time investigating such bugs then just version locking everything.

I thought requires files had version info?

[Jonathan Bedard]

(In reply to Stephanie Lewis from comment #8)
> Comment on attachment 406474 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406474&action=review
> 
> >>> Tools/Scripts/webkitpy/__init__.py:31
> >>> +AutoInstall.register(Package('toml', Version(0, 10, 1)))
> >> 
> >> Still don't like not finding dependencies automatically.  Going to add a lot of extraneous code.  Why are we not using requires files?
> > 
> > The biggest issue with parsing requires files is that older bots will have different library versions installed than newer ones.
> > 
> > This isn't totally theoretical. We had an issue a few months back with certifi (a dependency of requests), where newer bots didn't have the issue, but older ones did (because they had an older version of certifi). The solution to this is to version lock the dependencies, which leaves us back at listing out dependencies and their versions. We could pick and choose the dependencies we version lock, but the issue with this is that we won't know which dependencies we need to version lock until we encounter a bug, and I feel like we'd spend more time investigating such bugs then just version locking everything.
> 
> I thought requires files had version info?

They do, but most packages don't version lock their dependencies in requires files unless they need to. For example, requests specifies that certifi>=2017.4.17.  Most package installers (including the AutoInstaller) will attempt to use the newest version of certifi that meets this requirement. Meaning that when a new version of certifi is released, older machines will continue to use the old version, while newer machines will install the latest.

[Stephanie Lewis]

Comment on attachment 406474 [details]
Patch

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

>>>>> Tools/Scripts/webkitpy/__init__.py:31
>>>>> +AutoInstall.register(Package('toml', Version(0, 10, 1)))
>>>> 
>>>> Still don't like not finding dependencies automatically.  Going to add a lot of extraneous code.  Why are we not using requires files?
>>> 
>>> The biggest issue with parsing requires files is that older bots will have different library versions installed than newer ones.
>>> 
>>> This isn't totally theoretical. We had an issue a few months back with certifi (a dependency of requests), where newer bots didn't have the issue, but older ones did (because they had an older version of certifi). The solution to this is to version lock the dependencies, which leaves us back at listing out dependencies and their versions. We could pick and choose the dependencies we version lock, but the issue with this is that we won't know which dependencies we need to version lock until we encounter a bug, and I feel like we'd spend more time investigating such bugs then just version locking everything.
>> 
>> I thought requires files had version info?
> 
> They do, but most packages don't version lock their dependencies in requires files unless they need to. For example, requests specifies that certifi>=2017.4.17.  Most package installers (including the AutoInstaller) will attempt to use the newest version of certifi that meets this requirement. Meaning that when a new version of certifi is released, older machines will continue to use the old version, while newer machines will install the latest.

I'm still not seeing the problem. If we require a newer recertifi then we should specify that in our requirements file.  I don't think we should be recreating our own version of it.

[Jonathan Bedard]

(In reply to Stephanie Lewis from comment #10)
> ...
> 
> I'm still not seeing the problem. If we require a newer recertifi then we
> should specify that in our requirements file.  I don't think we should be
> recreating our own version of it.

That's essentially what we're doing, but recall, webkitpy doesn't actually have a requirements.txt file, that job has always been handled by the autoinstaller.

My point with bringing up certifi is that regardless of how we're version locking libraries, I am of the opinion that we should be version locking every single dependency, meaning that (to bring up the original point) we should not make an attempt to automatically parse the requirements files of the libraries we're using, since we should be version locking all of those dependencies.

Kocsen also had an idea where we would create a script which would dump the current versions for every dependency a particular library. That wouldn't be hard to do, and might be the right solution to the "we don't like figuring out what dependencies are"

[Jonathan Bedard]

Comment on attachment 406474 [details]
Patch

Bots were all happy with <https://trac.webkit.org/changeset/265574/webkit>, so landing this one too.

[EWS]

Committed r265607: <https://trac.webkit.org/changeset/265607>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 406474 [details].

[Stephanie Lewis]

(In reply to Jonathan Bedard from comment #11)
> (In reply to Stephanie Lewis from comment #10)
> > ...
> > 
> > I'm still not seeing the problem. If we require a newer recertifi then we
> > should specify that in our requirements file.  I don't think we should be
> > recreating our own version of it.
> 
> That's essentially what we're doing, but recall, webkitpy doesn't actually
> have a requirements.txt file, that job has always been handled by the
> autoinstaller.
> 
> My point with bringing up certifi is that regardless of how we're version
> locking libraries, I am of the opinion that we should be version locking
> every single dependency, meaning that (to bring up the original point) we
> should not make an attempt to automatically parse the requirements files of
> the libraries we're using, since we should be version locking all of those
> dependencies.
> 
> Kocsen also had an idea where we would create a script which would dump the
> current versions for every dependency a particular library. That wouldn't be
> hard to do, and might be the right solution to the "we don't like figuring
> out what dependencies are"

My point was that webkit should have a requirements.txt. We're creating our own syntax for specifying versions in a place that no one would look for with a syntax that would be custom to webkit.  I'm still not sure we shouldn't just be using pip and in this case I don't see any benefit to having our own custom requirements syntax.

[Jonathan Bedard]

(In reply to Stephanie Lewis from comment #14)
> (In reply to Jonathan Bedard from comment #11)
> > ...
> 
> My point was that webkit should have a requirements.txt. We're creating our
> own syntax for specifying versions in a place that no one would look for
> with a syntax that would be custom to webkit.  I'm still not sure we
> shouldn't just be using pip and in this case I don't see any benefit to
> having our own custom requirements syntax.

The issue with a raw requirements file (and pip) is that we would need some sort of setup script to configure one's development environment, and it's not even obvious where such a requirements script would live. Our python scripts wouldn't "just work", the user would need to pre-install our dependencies.

It's also worth mentioning that pip itself may be broken on the platform being tested (as it is now on Big Sur). Even when pip isn't broken, we would have manage the version of pip we're using a different way than all our other packages, since pip is managing every other package's version.

Ultimately, I think the desire for a "simpler" approach using pip and requirements files ends up with a system that has many more moving parts than an autoinstaller, where every python dependency that isn't checked in is managed the same way and everything "just works" for our developers

[Stephanie Lewis]

(In reply to Jonathan Bedard from comment #15)
> (In reply to Stephanie Lewis from comment #14)
> > (In reply to Jonathan Bedard from comment #11)
> > > ...
> > 
> > My point was that webkit should have a requirements.txt. We're creating our
> > own syntax for specifying versions in a place that no one would look for
> > with a syntax that would be custom to webkit.  I'm still not sure we
> > shouldn't just be using pip and in this case I don't see any benefit to
> > having our own custom requirements syntax.
> 
> The issue with a raw requirements file (and pip) is that we would need some
> sort of setup script to configure one's development environment, and it's
> not even obvious where such a requirements script would live. Our python
> scripts wouldn't "just work", the user would need to pre-install our
> dependencies.
> 
> It's also worth mentioning that pip itself may be broken on the platform
> being tested (as it is now on Big Sur). Even when pip isn't broken, we would
> have manage the version of pip we're using a different way than all our
> other packages, since pip is managing every other package's version.
> 
> Ultimately, I think the desire for a "simpler" approach using pip and
> requirements files ends up with a system that has many more moving parts
> than an autoinstaller, where every python dependency that isn't checked in
> is managed the same way and everything "just works" for our developers

Thank you for being patient to explain all this. That description seems to me like a reasonable reason not to use pip but it's still not clear to me why we wouldn't specify our dependencies in a requirement file and have the autoinstaller consume that rather than come up with our workflow for specifying dependencies

[Jonathan Bedard]

(In reply to Stephanie Lewis from comment #16)
> (In reply to Jonathan Bedard from comment #15)
> > ...
> 
> Thank you for being patient to explain all this. That description seems to
> me like a reasonable reason not to use pip but it's still not clear to me
> why we wouldn't specify our dependencies in a requirement file and have the
> autoinstaller consume that rather than come up with our workflow for
> specifying dependencies

Would be fine with that, although I need to familiarize myself with the syntax of requirement files when we have complicated things to say (for example, "if I'm on Windows, I want x, version, otherwise I want y version). I know this is possible, at least to some extent, I just don't know the syntax.

Although they're non-standard, I figured that whenever anyone reads Autoinstall declarations, they can pretty easily figure out what is happening, although we are definitely sacrificing some of the "why".