-
-
Notifications
You must be signed in to change notification settings - Fork 631
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate BinaryUtil options to bootstrap options. #4846
Migrate BinaryUtil options to bootstrap options. #4846
Conversation
@@ -53,27 +53,18 @@ class Factory(Subsystem): | |||
options_scope = 'binaries' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presumably the Factory should no longer extend Subsystem, and should not have an options_scope member.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BinaryUtil
still functions as a Subsystem
in all the existing call sites - that part isn't changing, so it should still extend Subsystem
afaict.
and removing the options_scope
on the class produces the following traceback:
Exception caught: (<type 'exceptions.TypeError'>)
File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_loader.py", line 73, in <module>
main()
File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_loader.py", line 69, in main
PantsLoader.run()
File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_loader.py", line 65, in run
cls.load_and_execute(entrypoint)
File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_loader.py", line 58, in load_and_execute
entrypoint_main()
File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_exe.py", line 26, in main
PantsRunner(exiter).run()
File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_runner.py", line 57, in run
options_bootstrapper=options_bootstrapper)
File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_runner.py", line 46, in _run
return LocalPantsRunner(exiter, args, env, options_bootstrapper=options_bootstrapper).run()
File "/Users/kwilson/dev/pants/src/python/pants/bin/local_pants_runner.py", line 37, in run
self._run()
File "/Users/kwilson/dev/pants/src/python/pants/bin/local_pants_runner.py", line 43, in _run
options, build_config = OptionsInitializer(options_bootstrapper, exiter=self._exiter).setup()
File "/Users/kwilson/dev/pants/src/python/pants/init/options_initializer.py", line 161, in setup
options = self._install_options(self._options_bootstrapper, build_configuration)
File "/Users/kwilson/dev/pants/src/python/pants/init/options_initializer.py", line 115, in _install_options
known_scope_infos.append(subsystem.get_scope_info())
File "/Users/kwilson/dev/pants/src/python/pants/subsystem/subsystem.py", line 80, in get_scope_info
cls.validate_scope_name_component(cls.options_scope)
File "/Users/kwilson/dev/pants/src/python/pants/option/optionable.py", line 39, in validate_scope_name_component
if not cls.is_valid_scope_name_component(s):
File "/Users/kwilson/dev/pants/src/python/pants/option/optionable.py", line 35, in is_valid_scope_name_component
return cls._scope_name_component_re.match(s) is not None
Exception message: expected string or buffer
so I left it in + thought having an explicit namespace would help make it clearer which options in the bootstrap options were related. let me know if you see a better way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's a subsystem then it definitely needs an options_scope, but it seems like a better idea if it could not be one any more...
AFAICT the callsites do BinaryUtil.Factory.create()
to get an instance, and that needn't change if we make Factory no longer extend Subsystem. Would that break in some way I'm not seeing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would that break in some way I'm not seeing?
yes, during the call to cls.global_instance().get_options()
to get the bootstrap options - unless we want to explicitly thread an options context into every call.
BinaryUtil.Factory
is also :API: Public
and widely in use in private plugins afaik, so we couldn't change that directly without a new create
method + a deprecation slog. I think it's still perfectly reasonable to continue to address BinaryUtil
as a subsystem post-subsystem initialization. this change just permits us to utilize BinaryUtil
prior to subsystem initialization with only bootstrap options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just don't love that it has its own options scope, even though it's not allowed to have its own options. That seems dirty. Do we add a comment saying "do not register options here", or do we allow that, with the understanding that those options will only have their default values during initialization?
I suppose that's not horrible. We know exactly what we need BinaryUtil for during initialization, so it may have options in the future that are irrelevant to our needs, but relevant to those of tasks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems like a bi-product of the Subsystem
design tho - if Subsystem
would relax that requirement, we could indeed avoid the (currently unused) options scope. right now, it's required - but doesn't seem strictly necessary (case in point).
will add a quick comment to clarify for now.
CI is timing out on integration test shard 1, but given the prior green CI + comment-only change I'm going to merge this under the assumption that the real problem is #4850 |
Problem
In order to fully externalize pantsd lifecycle, we need to untangle pantsd's reliance on
Subsystem
dependencies and their options. This is an initial step in that direction.Solution
Migrate
BinaryUtil
options to bootstrap options.Result
Able to leverage
BinaryUtil
before subsystems are setup by directly callingBinaryUtil.__init__
with global options.