Add an `--enable-engine` flag to leverage the v2 engine-backed LegacyBuildGraph without pantsd.

Review Request #3932 — Created May 25, 2016 and submitted

3465, 3499
benjyw, ity, jsirois, peiyu, stuhood, wisechengyi
  • Add an --enable-engine flag for easier manual and integration testing of the new engine/scheduler-backed LegacyBuildGraph without the daemon (all cold-cache runs).
  • Migrate integration tests to use of this flag and kill legacy/commands.
  • Test coverage and misc cleanup.

CI is away @

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
  2. if this takes time, add a logging statement before it

    1. it can take some time, but already has it's own internal logging at the start, e.g.:

      DEBUG] Injecting to <pants.engine.legacy.graph.LegacyBuildGraph object at 0x4105dda190>: [DescendantAddresses(directory='3rdparty')]
  3. typo: pants daemon?

    1. ah, no - "sans <X>" == "without <X>".

  2. src/python/pants/engine/ (Diff revision 1)

    It would be good to update EngineInitializer in this change, others as you see fit:

    $ git grep -n "_cache.get_stats()"
    src/python/pants/bin/      logger.debug('engine cache stats: {}'.format(engine._cache.get_stats()))
    src/python/pants/pantsd/service/    self._logger.debug('engine cache stats: %s', self._engine._cache.get_stats())
    tests/python/pants_test/engine/      cache_stats = engine._cache.get_stats()
    1. ah, good catch - fixed.

  1. Thanks!

  2. src/python/pants/bin/ (Diff revision 1)

    Should the cached graph should only be used if use_engine is true, since we can assume that's where it came from?

    1. currently the Pailgun runner is the only place where a cached_buildgraph is ever passed in - so the thinking is --enable-pantsd implicitly enables --enable-engine and thus we shouldn't need an extra check against --enable-engine - we just use any cached graphs that are passed in.

      clarified this a bit in the --enable-pantsd help string - let me know if that doesn't suffice.

  3. This should indicate that it requires/implies --enable-engine... or at the very least, overrides that setting.

  2. I feel this sounds ambiguous.
    maybe "enable-v2-engine"
    something like engine=v2, with

    1. changed this to --enable-v2-engine.

  1. thanks for doing this

  2. its unclear to users what would happen when enable-pantsd and enable-engine are both true

    1. I added a clarification per Stu's comments around --enable-pantsd implicitly enabling --enable-engine - let me know if that's not what you were getting at.

  3. could you add a test that asserts that daemon is off when running this?

    1. added an extra check here.

  1. Ship It!
Review request changed

Status: Closed (submitted)

Change Summary:

thanks for the reviews! this is in @