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

Split short-form from long-form of setup_legacy_graph #5911

Merged
merged 4 commits into from Jun 5, 2018

Conversation

Projects
None yet
4 participants
@stuhood
Copy link
Member

stuhood commented Jun 4, 2018

Problem

EngineInitializer.setup_legacy_graph takes 14 arguments, but the two "real" callsites for it were independently constructing those arguments from the bootstrap options. This lead to an issue where the two real callsites went out of sync (see #5897).

Solution

Rename the 14-arg form to *_extended, and give the shorter name to a 3-arg form that uses the bootstrap options. Additionally, clean-up/remove/explain callsites using the extended form.

Result

Fixes #5897 and avoids issues like it.

@stuhood stuhood requested review from benjyw , cosmicexplorer and kwlzn Jun 4, 2018

@benjyw

benjyw approved these changes Jun 5, 2018

@kwlzn

kwlzn approved these changes Jun 5, 2018

Copy link
Member

kwlzn left a comment

lgtm mod one comment about kwarg redaction

remote_execution_server=self._options.for_global_scope().remote_execution_server,
).new_session()
graph_scheduler_helper = EngineInitializer.setup_legacy_graph(native,
self._global_options)

This comment has been minimized.

@kwlzn

kwlzn Jun 5, 2018

Member

seems like several kwargs that were explicitly passed in before from options at this callsite (e.g. glob_match_error_behavior, rules, include_trace_on_error, build_file_aliases etc) are now omitted in the new call to setup_legacy_graph - is that expected?

This comment has been minimized.

@stuhood

stuhood Jun 5, 2018

Member

Good eye! Hence the CI failures. Thanks.

@stuhood stuhood force-pushed the twitter:stuhood/unify-engine-initializer-usages branch from 6a12dcf to 7175dd8 Jun 5, 2018

@stuhood stuhood merged commit cfc8d64 into pantsbuild:master Jun 5, 2018

1 check passed

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

@stuhood stuhood deleted the twitter:stuhood/unify-engine-initializer-usages branch Jun 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment