Bug 216396 – [webkitcorepy] Disable AutoInstaller with environment variable

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

[Jonathan Bedard]

Feature request from Dean Johnson to be able to disable the AutoInstaller with an environment variable.

[Radar WebKit Bug Importer]

<rdar://problem/68680933>

[Jonathan Bedard]

Created attachment 408508 [details]
Patch

[Dean Johnson]

Comment on attachment 408508 [details]
Patch

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

Unofficial r+ with some simplification of enable/disable.

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:300
> +    _ENVIRONMENT_VARIABLE = 'WEBKITCOREPY_AUTOINSTALL'

Nit: Can we make the name of the variable and env far more descriptive? e.g.
_ENVIRONMENT_VARIABLE -> ENABLED_ENV_VAR
WEBKITCOREPY_AUTOINSTALL -> DISABLE_WEBKITCOREPY_AUTOINSTALLER

It would also require inverting the checks below, but since the default for the auto installer is "enabled" I think including "disable" in the envvar is important.

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:303
>      enabled = False

Why does this value default to False if it's actually enabled when it's imported/used?

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:335
>      def enable(cls):

I think it'd be cleaner to just use one function / check for this and name it 'enabled'. I can't really think of a use-case where you'd want to enable, then disable, in the same runtime (other than testing, in which case you can mock).

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:336
> +        if cls._enabled_variable() is False:

An example of this is:
```
@classmethod
def enabled(cls):
    if os.environ.get(cls._ENVIRONMENT_VARIABLE) in ['0', 'FALSE', 'false', 'NO', 'no']:
        return False
    return True
```

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:387
> +        if cls._enabled_variable() is False:

Nit: I found in local testing that doing an early return in AutoInstaller.register allowed me to skip all of the auto installer, does this accomplish the same thing? I think it might be better to do the check in `register` since it's the function most likely to be first looked at by a client when looking into issues. This is just a preference though and doesn't need to change.

[Jonathan Bedard]

Comment on attachment 408508 [details]
Patch

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

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:303
>>      enabled = False
> 
> Why does this value default to False if it's actually enabled when it's imported/used?

Because the AutoInstaller needs to have a target directory set before it's enabled. webkitpy enables the Autoinstaller when it's imported, but that's a decision that's up to our client programs.

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:335
>>      def enable(cls):
> 
> I think it'd be cleaner to just use one function / check for this and name it 'enabled'. I can't really think of a use-case where you'd want to enable, then disable, in the same runtime (other than testing, in which case you can mock).

Good point, I think removing "enable" and "disable" will go a long way to clarifying this patch.

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:387
>> +        if cls._enabled_variable() is False:
> 
> Nit: I found in local testing that doing an early return in AutoInstaller.register allowed me to skip all of the auto installer, does this accomplish the same thing? I think it might be better to do the check in `register` since it's the function most likely to be first looked at by a client when looking into issues. This is just a preference though and doesn't need to change.

Registered packages won't be installed if we don't have a directory to install them to. I think we still want to be able to iterate through all packages the AutoInstaller has registered for a give script or configuration. Among other things, it would let us generate a version locked requirements.txt.

[Jonathan Bedard]

Created attachment 408559 [details]
Patch

[Dean Johnson]

Comment on attachment 408559 [details]
Patch

Unofficial r+; LGTM. Thanks for doing this!

Comment on attachment 408559 [details]
Patch

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

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:326
> +        if os.environ.get(cls.DISABLE_ENV_VAR) not in ['0', 'FALSE', 'false', 'NO', 'no', None]:

Should we also consider to add 'No' and 'False' which seem quite common as well?

[Jonathan Bedard]

(In reply to dewei_zhu from comment #7)
> Comment on attachment 408559 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408559&action=review
> 
> > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:326
> > +        if os.environ.get(cls.DISABLE_ENV_VAR) not in ['0', 'FALSE', 'false', 'NO', 'no', None]:
> 
> Should we also consider to add 'No' and 'False' which seem quite common as
> well?

Will add! Also, waiting on https://bugs.webkit.org/show_bug.cgi?id=216480 so that we can log everything in the same way.

[EWS]

Committed r267019: <https://trac.webkit.org/changeset/267019>

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

[Jonathan Bedard]

(In reply to EWS from comment #9)
> Committed r267019: <https://trac.webkit.org/changeset/267019>
> 
> All reviewed patches have been landed. Closing bug and clearing flags on
> attachment 408559 [details].

Oops, meant to land https://bugs.webkit.org/show_bug.cgi?id=216480 first, going to need a follow-up fix here....

[Jonathan Bedard]

Reopening to attach new patch.

[Jonathan Bedard]

Created attachment 408728 [details]
Patch

[EWS]

Committed r267035: <https://trac.webkit.org/changeset/267035>

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