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

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

kwlzn
pants
3465, 3499
pants-reviews
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 @ https://travis-ci.org/pantsbuild/pants/builds/132776390

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
PE
  1. 
      
  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>".

  4. 
      
JS
  1. 
      
  2. src/python/pants/engine/engine.py (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/engine_initializer.py:131:      logger.debug('engine cache stats: {}'.format(engine._cache.get_stats()))
    src/python/pants/pantsd/service/scheduler_service.py:89:    self._logger.debug('engine cache stats: %s', self._engine._cache.get_stats())
    tests/python/pants_test/engine/test_engine.py:68:      cache_stats = engine._cache.get_stats()
    
    1. ah, good catch - fixed.

  3. 
      
ST
  1. Thanks!

  2. src/python/pants/bin/goal_runner.py (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.

    1. clarified this.

  4. 
      
YU
  1. 
      
  2. I feel this sounds ambiguous.
    maybe "enable-v2-engine"
    or
    something like engine=v2, with
    choices=['v1','v2']?

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

  3. 
      
IT
  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.

  4. 
      
KW
YU
  1. Ship It!
  2. 
      
KW
Review request changed

Status: Closed (submitted)

Change Summary:

thanks for the reviews! this is in @ https://github.com/pantsbuild/pants/commit/60cb4230104a6cd6de1e9c9dde2038a0088f1a66

Loading...