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

test --enable-v2-engine (as a global default) in master #3740

Closed
kwlzn opened this issue Jul 29, 2016 · 14 comments
Closed

test --enable-v2-engine (as a global default) in master #3740

kwlzn opened this issue Jul 29, 2016 · 14 comments
Assignees

Comments

@kwlzn
Copy link
Member

kwlzn commented Jul 29, 2016

in order to discover unknown bugs in areas of pants that are untested by our internal CI sandboxing, we'll also want to run all of the pants test with this flag enabled.

there are a few approaches to this:

  1. expand the usage of the @ensure_engine decorator (whitelist approach)
  2. enable the behavior of @ensure_engine by default and invert the whitelisting approach in twitter-commons merge into twitter #1 to except specific tests (blacklist approach)
  3. enable a completely separate jenkins job/configuration to mirror CI with --enable-v2-engine as default configuration

additionally, initial smoke testing via PR may prove useful in uncovering an initial survey of the state of things - as well as informing the initial round of new bugs shaken out from this.

@kwlzn kwlzn added the engine label Jul 29, 2016
@kwlzn kwlzn added this to the v2 engine adoption milestone Jul 29, 2016
@JieGhost
Copy link
Contributor

PR is here:
#3770

@JieGhost
Copy link
Contributor

JieGhost commented Aug 10, 2016

Initial run findings:

these are the test failures I can repro on my local machine and if using v1 engine, they all pass.
./pants test tests/python/pants_test/build_graph:build_graph_integration
./pants test tests/python/pants_test/core_tasks:deferred_sources_mapper_integration
./pants test tests/python/pants_test/core_tasks:prep_command_integration
./pants test tests/python/pants_test/projects:examples_integration
./pants test tests/python/pants_test/projects:testprojects_integration
./pants test tests/python/pants_test/base:ignore_patterns_pants_ini_integration
./pants test tests/python/pants_test/backend/project_info/tasks:export_integration
./pants test tests/python/pants_test/backend/project_info/tasks:idea_integration

I will examine them and see if new tickets need to be created.

@JieGhost
Copy link
Contributor

v2 engine uses project_tree which uses pants_ignore.

./pants test tests/python/pants_test/core_tasks:deferred_sources_mapper_integration
./pants test tests/python/pants_test/core_tasks:prep_command_integration

the above 2 tests have BUILD in a tmp dir under .pants.d, with default pants_ignore setting, .pants.d will be ignored thus no build files can be found.

set ignore_pattern to be [] in config override.

"./pants test tests/python/pants_test/core_tasks:prep_command_integration" succeeded.

A new error from:
"./pants test tests/python/pants_test/core_tasks:deferred_sources_mapper_integration"

@JieGhost
Copy link
Contributor

These are the targets that fail with v2 engine at the moment:

./pants test tests/python/pants_test/build_graph:build_graph_integration
./pants test tests/python/pants_test/core_tasks:deferred_sources_mapper_integration
./pants test tests/python/pants_test/projects:examples_integration
./pants test tests/python/pants_test/projects:testprojects_integration
./pants test tests/python/pants_test/base:ignore_patterns_pants_ini_integration
./pants test tests/python/pants_test/backend/project_info/tasks:export_integration
./pants test tests/python/pants_test/backend/project_info/tasks:idea_integration

@JieGhost
Copy link
Contributor

JieGhost commented Aug 11, 2016

The following three have similar exceptions, all complaining TargetAdaptor not having certain attributes.
./pants test tests/python/pants_test/core_tasks:deferred_sources_mapper_integration
./pants test tests/python/pants_test/projects:examples_integration
./pants test tests/python/pants_test/backend/project_info/tasks:idea_integration

They all have codegen tasks and TargetAdaptor is created as synthetic target. Normally synthetic target at codegen stage should not be of type TargetAdaptor. I guess it's due to the symbol table buildgraph is using.

Investigating...

@JieGhost
Copy link
Contributor

RemoteSources target has a field called "dest" which is the destination target type. In v2 engine, during parsing, all target type is converted to TargetAdaptor (including JvmAppAdaptor and PythonTargetAdaptor). Thus "dest" field is converted to TargetAdaptor as well. When instantiating TargetAdaptor to normal target type, "dest" field is not converted, thus trigger exception.

I added a special handling for RemoteSources target during target instantiating. Now these 2 passes:
./pants test tests/python/pants_test/core_tasks:deferred_sources_mapper_integration
./pants test tests/python/pants_test/projects:examples_integration

This one has a new error:
./pants test tests/python/pants_test/backend/project_info/tasks:idea_integration

@JieGhost
Copy link
Contributor

These are the targets that fail with v2 engine at the moment:

./pants test tests/python/pants_test/build_graph:build_graph_integration
./pants test tests/python/pants_test/projects:testprojects_integration
./pants test tests/python/pants_test/base:ignore_patterns_pants_ini_integration
./pants test tests/python/pants_test/backend/project_info/tasks:export_integration
./pants test tests/python/pants_test/backend/project_info/tasks:idea_integration

@JieGhost
Copy link
Contributor

These 3 tests fail due to AddressMapper used in v2 engine:

./pants test tests/python/pants_test/projects:testprojects_integration
./pants test tests/python/pants_test/backend/project_info/tasks:export_integration
exclude_target_regexp ignored in v2 engine.

./pants test tests/python/pants_test/base:ignore_patterns_pants_ini_integration
build_ignore_patterns ignored in v2 engine.

@JieGhost
Copy link
Contributor

this one fails because error messages are different when detecting a cycle in build graph between v1 and v2 engine.

./pants test tests/python/pants_test/build_graph:build_graph_integration

@JieGhost
Copy link
Contributor

All 5 remaining test failures have been fixed. Once changes are in master, I will start a new round of PR.

@JieGhost
Copy link
Contributor

With the latest patch (not in master yet, https://rbcommons.com/s/twitter/r/4239/), PR failed only in 1 shard, with the still weird "import error (module not found)". I have noticed this error happened randomly on different shards, so I will run ci several times to get some data.

@JieGhost
Copy link
Contributor

All integration tests can pass on v2 engine with python implementation.

@JieGhost
Copy link
Contributor

I kicked off a new run of integration tests on v2 engine since now v2 engine is using rust backend. I have seen 4 failures in the first run, 3 of them are about certain strings not found in exception messages. I think it is related to #4007

@stuhood stuhood changed the title test --enable-v2-engine (as a global default) in master Make --enable-v2-engine the default in master Jan 10, 2017
@stuhood stuhood changed the title Make --enable-v2-engine the default in master test --enable-v2-engine (as a global default) in master Jan 10, 2017
@stuhood
Copy link
Member

stuhood commented Jan 10, 2017

Thanks for all your work here @JieGhost! I think the next step will be to work to change the default for the flag. Opened #4173 for that.

@stuhood stuhood closed this as completed Jan 10, 2017
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

No branches or pull requests

3 participants