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

[engine] Check for duplicate deps in v2 graph construction. #4616

Merged
merged 1 commit into from May 23, 2017

Conversation

Projects
None yet
4 participants
@kwlzn
Copy link
Member

kwlzn commented May 23, 2017

Fixes #4613

Problem

Currently, the v2 path does not safeguard against duplicate dependencies listed in BUILD files.

Solution

Check for duplicates.

Result

[omerta pants (kwlzn/dupe_deps)]$ cat BUILD
target(name='world')

target(name='test',
  dependencies=[
    ':world',
    ':world'
  ]
)
[omerta pants (kwlzn/dupe_deps)]$ ./pants list :
Exception caught: (<class 'pants.build_graph.build_graph.DuplicateAddressError'>)
  File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_exe.py", line 50, in <module>
    main()
  File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_exe.py", line 44, 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 77, in _run
    self._exiter).setup()
  File "/Users/kwilson/dev/pants/src/python/pants/bin/goal_runner.py", line 199, in setup
    goals, context = self._setup_context(pantsd_launcher)
  File "/Users/kwilson/dev/pants/src/python/pants/bin/goal_runner.py", line 171, in _setup_context
    self._global_options.subproject_roots,
  File "/Users/kwilson/dev/pants/src/python/pants/bin/goal_runner.py", line 113, in _init_graph
    include_trace_on_error=self._global_options.print_exception_stacktrace)
  File "/Users/kwilson/dev/pants/src/python/pants/bin/engine_initializer.py", line 101, in create_build_graph
    for _ in graph.inject_specs_closure(target_roots.as_specs()):
  File "/Users/kwilson/dev/pants/src/python/pants/engine/legacy/graph.py", line 233, in inject_specs_closure
    for address in self._inject(specs):
  File "/Users/kwilson/dev/pants/src/python/pants/engine/legacy/graph.py", line 253, in _inject
    self._index(target_entries)
  File "/Users/kwilson/dev/pants/src/python/pants/engine/legacy/graph.py", line 103, in _index
    new_targets.append(self._index_target(target_adaptor))
  File "/Users/kwilson/dev/pants/src/python/pants/engine/legacy/graph.py", line 152, in _index_target
    .format(spec=dependency.spec, target=address.spec)

Exception message: Addresses in dependencies must be unique. '//:world' is referenced more than once by target '//:test'.

@kwlzn kwlzn changed the title Check for duplicate deps in v2 graph construction. [engine] Check for duplicate deps in v2 graph construction. May 23, 2017

@stuhood stuhood added this to the 1.3.0 milestone May 23, 2017

@stuhood
Copy link
Member

stuhood left a comment

Thanks Kris. I'm fine without the test here.

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented May 23, 2017

Could you add a regression test? As this is an end-user actionable error, I'd like a test for it.

@kwlzn

This comment has been minimized.

Copy link
Member

kwlzn commented May 23, 2017

@baroquebobcat I added a note in #4613, but adding coverage here would just be obsoleted by #4401 (after which this will be covered by the existing test in test_build_graph.py). so it'd essentially be throwaway work that would require solving for the current blocker in #4401 afaict. still think this is necessary?

@baroquebobcat

This comment has been minimized.

Copy link
Contributor

baroquebobcat commented May 23, 2017

Sorry. I missed that comment. If it'll be covered by converting the test_build_graph tests, I'm ok with it.

@ity

ity approved these changes May 23, 2017

Copy link
Member

ity left a comment

lgtm!

@kwlzn kwlzn force-pushed the twitter:kwlzn/dupe_deps branch from a70ccb4 to c6598fd May 23, 2017

@kwlzn kwlzn merged commit 879a314 into pantsbuild:master May 23, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@kwlzn

This comment has been minimized.

Copy link
Member

kwlzn commented May 23, 2017

the red ci here appears flaky - bintray failures. merging.

stuhood added a commit that referenced this pull request May 23, 2017

[engine] Check for duplicate deps in v2 graph construction. (#4616)
Fixes #4613

Problem

Currently, the v2 path does not safeguard against duplicate dependencies listed in BUILD files.

Solution

Check for duplicates.

Result

[omerta pants (kwlzn/dupe_deps)]$ cat BUILD
target(name='world')

target(name='test',
  dependencies=[
    ':world',
    ':world'
  ]
)
[omerta pants (kwlzn/dupe_deps)]$ ./pants list :
Exception caught: (<class 'pants.build_graph.build_graph.DuplicateAddressError'>)
  File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_exe.py", line 50, in <module>
    main()
  File "/Users/kwilson/dev/pants/src/python/pants/bin/pants_exe.py", line 44, 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 77, in _run
    self._exiter).setup()
  File "/Users/kwilson/dev/pants/src/python/pants/bin/goal_runner.py", line 199, in setup
    goals, context = self._setup_context(pantsd_launcher)
  File "/Users/kwilson/dev/pants/src/python/pants/bin/goal_runner.py", line 171, in _setup_context
    self._global_options.subproject_roots,
  File "/Users/kwilson/dev/pants/src/python/pants/bin/goal_runner.py", line 113, in _init_graph
    include_trace_on_error=self._global_options.print_exception_stacktrace)
  File "/Users/kwilson/dev/pants/src/python/pants/bin/engine_initializer.py", line 101, in create_build_graph
    for _ in graph.inject_specs_closure(target_roots.as_specs()):
  File "/Users/kwilson/dev/pants/src/python/pants/engine/legacy/graph.py", line 233, in inject_specs_closure
    for address in self._inject(specs):
  File "/Users/kwilson/dev/pants/src/python/pants/engine/legacy/graph.py", line 253, in _inject
    self._index(target_entries)
  File "/Users/kwilson/dev/pants/src/python/pants/engine/legacy/graph.py", line 103, in _index
    new_targets.append(self._index_target(target_adaptor))
  File "/Users/kwilson/dev/pants/src/python/pants/engine/legacy/graph.py", line 152, in _index_target
    .format(spec=dependency.spec, target=address.spec)

Exception message: Addresses in dependencies must be unique. '//:world' is referenced more than once by target '//:test'.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment