Skip to content
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

Implement rules that directly produce Subsystems from options #5869

Closed
stuhood opened this issue May 25, 2018 · 3 comments · Fixed by #6872
Closed

Implement rules that directly produce Subsystems from options #5869

stuhood opened this issue May 25, 2018 · 3 comments · Fixed by #6872
Assignees

Comments

@stuhood
Copy link
Member

stuhood commented May 25, 2018

This depends on #5788, #5831, and #6478. With the prerequisites in place, we should be able to write rules that accept environment variables and the contents of files outside the buildroot as Params that propagate down into subgraphs.

This will likely look like using the "multi-parameter-Get" support from #6478 to additionally pass in a (renamed?) OptionsParseRequest as a Param when a @console_rule is executed. This will make the OptionsParseRequest available anywhere in the subgraph below the @console_rule.

We should expand the rules in options_parsing.py to expose scoped options which could be requested by @rule s that construct Subsystems... ie, something like:

@rule(ScopedOptions, [Select(Scope), Select(Options)]

...which would transitively consume the OptionsParseRequest, and which a Subsystem-constructing rule could consume using:

@rule(MyCoolSubsystem, [])
def cool_subsystem():
  scoped_options = yield Get(ScopedOptions, Scope('cool.scope'))
  yield MyCoolSubsystem(arg1=scoped_options.arg1, ..)
@stuhood
Copy link
Member Author

stuhood commented Nov 12, 2018

For reference: I think that this is still blocked on #6478.

@stuhood
Copy link
Member Author

stuhood commented Nov 28, 2018

I looked into this a bit today, and the components of the work look fairly clear:

  1. Since they will be involved in the identity of Nodes that consume options, we need to tighten up __eq__ for OptionsBootstrapper and BuildConfiguration.
  2. Move target_spec_files consumption into the construction of the OptionsBootstrapper so that all direct file consumption happens during bootstrap (this punts on supporting reading arbitrary files during options parsing).
  3. Add a ConsoleRequest that holds the OptionsBootstrapper and BuildConfiguration to be used as the root subject when running @console_rules, and a series of @rules that use the existing rule in options_parsing.py to construct a ScopedOptions datatype (a filtered copy of Options).
  4. Adjust Subsystem creation to support construction from ScopedOptions, and expose one Subsystem in this way.

Notably, because 1) all file parsing will happen during construction of the OptionsBootstrapper, 2) we only have a single root subject (the "ConsoleRequest"), this ticket does not seem to be blocked on #6478!


Some rough notes follow.

LocalPantsRunner.parse_options
  OptionsBootstrapper.__init__
    # noop
  OptionsBootstrapper.get_bootstrap_options()
    OptionsBootstrapper.(construct_and_set_bootstrap_options|produce_and_set_bootstrap_options)
      # most likely reasonable to re-parse and then inject for every run.
      # so we'd pass down an OptionsBootstrapper + BuildConfiguration.
  BuildConfigInitializer.get(options_bootstrapper)
    # only necessary when bootstrap options have changed: defines which rules are loaded
    BuildConfigInitializer.setup
      # cached: very sideeffecty, very expensive.
  OptionsInitializer.create
    # not cached
    OptionsInitializer._construct_options
      OptionsBootstrapper.get_full_options
        Options.create
          # always parses all options

@stuhood
Copy link
Member Author

stuhood commented Dec 3, 2018

I got started here last week, and while it's true that it would be possible to accomplish this without #6478, I noticed that there might be a shorter path available if we actually implement #6478 before diving all the way in here.

I started by making OptionsBootstrapper into a datatype, and then realized that because the output of options parsing was an Options object, we would need useful equality there too. I implemented equality for Options, and then realized that because both the OptionsBootstrapper and BuildConfiguration must be created for every run, only the Options object actually needs to be injected into the graph.

But going back even further: since we're already re-reading all config files each time we run (in order to bootstrap), we can actually just inject the Options value itself. Then (optionally using #6845), we can have rules produce the Subsystems given the Options value. See #6485 for a discussion of how to make this "somewhat" efficient, although there are open questions.

@stuhood stuhood added the blocked label Dec 3, 2018
@stuhood stuhood removed the blocked label Dec 5, 2018
stuhood pushed a commit that referenced this issue Dec 6, 2018
### Problem

As described in #5869, we need a way to consume `Options` in the new engine in order to provide access to `Subsystems` in `@rules`. 

### Solution

Convert `OptionsBootstrapper` and `Config` into `datatype`s in order to give them useful eq/hash implementations.

Then, modify the `@rules` that were added in #5889 to consume the `Params` API added in #6871 and expose a `ScopedOptions` wrapper around the set of options for a `Scope`.

### Result

`@rules` can consume `ScopedOptions`, although there should probably be built-in rules that can consume `ScopedOptions` to create `Subsystems`. Nonetheless, let's say that this "fixes #5869", and open a followup for actually exposing `Subsystems` to `@rules` in a generic way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant