Init the native engine from bootstrap options.#4787
Conversation
de1973c to
90a78c9
Compare
stuhood
left a comment
There was a problem hiding this comment.
Thanks Benjy!
Before shipping this, would be good to talk to Kris about the "engine as native wheel" option that he had mentioned... it would remove the use of binutils entirely.
| @@ -129,8 +129,8 @@ function bootstrap_native_code() { | |||
| cp "${native_binary}" "${target_binary}" | |||
|
|
|||
| # NB: The resource file emitted/over-written below is used by the `Native` subsystem to default | |||
|
|
||
|
|
||
| # TODO: These aren't tests, so they should be under examples/. | ||
| # Should they use |
There was a problem hiding this comment.
They are used in the tests, so maybe testprojects/ instead.
"// TODO: finish this tho"
There was a problem hiding this comment.
This module was completely broken before I fixed it up, using a method that no longer exists. So nothing is currently exercising this code. And I don't see any references to visualizer anywhere else in the codebase.
90a78c9 to
2e58d5c
Compare
Previously the native engine was initialized by a Native.Factory subsystem. However we want to use the engine to parse options, meaning that the engine must be initialized using only bootstrap options. So the subsystem options were moved to the bootstrap options (their cmd-line and env names don't change, because we use the old subsystem scope as a prefix for the new global option names, however they will need to move to the global section in pants.ini). The code was moved out of pants/engine/subsystem into pants/engine directly. We could have renamed the subsystem directory to something else, but the relevant target both depends on and is depended on by targets in the parent dir, so it should probably just live there (circular deps at the directory level are icky).
2e58d5c to
bc6e892
Compare
kwlzn
left a comment
There was a problem hiding this comment.
lgtm, tho I thought we were generally against passing around options "bags" to __init__ methods as is done here for PantsDaemonLauncher. doesn't matter to me either way tho.
|
I don't remember any discussion of passing around option bags, so I think it's fine, and I think this is better than passing in so many separate option fields... |
Previously the native engine was initialized by a
Native.Factory subsystem. But we want to use the engine
to parse options, meaning that the engine must be initialized
using only bootstrap options.
So this change moves the former subsystem options to be bootstrap
options. We use the old subsystem scope as a prefix for the new global
option names, so their cmd-line and env names don't change, however any
settings in pants.ini will need to move to the global section.
The code was moved out of
pants/engine/subsystemintopants/enginedirectly. We could have renamed the subsystemdirectory to something else, but the relevant target both
depends on and is depended on by targets in the parent dir,
so it should probably just live there (circular deps at the
directory level are icky).
A future change will give BinaryUtil.Factory similar treatment, clearing
the way for initializing the native engine using only bootstrap options.