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

Expose an API to pass multiple Params to an engine request #6871

Merged
merged 4 commits into from Dec 6, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Dec 5, 2018

Problem

Although consumption of multiple Params is now supported, there is currently no way to actually provide them to a request. See #6478 for more info.

Solution

Expose and test the ability to pass in a small Params datatype as a subject, which is flattened into a corresponding set of rust-side Params in add_root_select.

Result

Fixes #6478.

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Dec 5, 2018

Commits should be useful to review independently.

@ity

ity approved these changes Dec 6, 2018

Copy link
Member

ity left a comment

I havent reviewed the rust part yet, but the python part is looking good! there are some legit test failures - one of which I highlighted in review.

Looks great!

This method should be called by exactly one scheduling thread, but the Step objects returned
by this method are intended to be executed in multiple threads, and then satisfied by the
scheduling thread.
:return: A tuple of (root, Return) tuples and (root, Throw) tuples.

This comment has been minimized.

@ity

ity Dec 6, 2018

Member

i havent seen this return format before, but makes it a lot cleaner!

Show resolved Hide resolved src/python/pants/engine/legacy/address_mapper.py
if returns:
state = returns[0][1]
else:
state = throws[0][1]

This comment has been minimized.

@wisechengyi

wisechengyi Dec 6, 2018

Contributor

Q:

  1. is it possible to have both returns and throws? in which case the logic seems to ignore throws.
  2. is there certain expectation on execute's return value, e.g. len(throws) + len(returns) = len(request.roots)?

This comment has been minimized.

@stuhood

stuhood Dec 6, 2018

Member

Yes to your second question, so in this case: no to your first question.

@stuhood stuhood merged commit 3fbb0e3 into pantsbuild:master Dec 6, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/expose-params-ux branch Dec 6, 2018

stuhood added a commit that referenced this pull request Dec 6, 2018

Add the ability to consume scoped Options from @rules (#6872)
### 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