Feature request from Dean Johnson to be able to disable the AutoInstaller with an environment variable.
<rdar://problem/68680933>
Created attachment 408508 [details] Patch
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.
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.
Created attachment 408559 [details] Patch
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?
(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.
Committed r267019: <https://trac.webkit.org/changeset/267019> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408559 [details].
(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....
Reopening to attach new patch.
Created attachment 408728 [details] Patch
Committed r267035: <https://trac.webkit.org/changeset/267035> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408728 [details].