Review Request 4342: Upgrade zinc's sbt dependency to 1.0.0: python portion

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Yi Cheng, and Eric Ayers. By Peiyu Wang. Repository: pants Description This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Testing Testing is done locally, will update with travis link as soon as https://rbcommons.com/s/twitter/r/4340 is shipped and published Diffs 3rdparty/jvm/org/openjdk/jmh/BUILD (PRE-CREATION) contrib/scalajs/src/python/pants/contrib/scalajs/targets/scala_js_target.py (1d14166cbe49064cc4ebba2c20a853873ac0bc02) src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (655b96a78645041afd9941400de89b1e72425b83) src/python/pants/backend/jvm/targets/jvm_target.py (98b0707bdbab6c7332726c58c08100ea0b51c0b7) src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (6d6ede8c2961c4bc349122042ee87eb3c93a5d30) src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (399b43cc90974a6e53bb892956b7c8587fb8e5b9) src/python/pants/backend/jvm/tasks/jvm_dependency_usage.py (a690491f3f0761808f6ebc67d192b544a0195158) src/python/pants/backend/jvm/zinc/zinc_analysis.py (0a0bfd5a0f036ea07712d2b9074b6bc93c3ab807) src/python/pants/backend/jvm/zinc/zinc_analysis_element_types.py (2d0d87615d5f29205117eb6b3d6ce3084ab3b1c3) src/python/pants/backend/jvm/zinc/zinc_analysis_parser.py (531f35ea03f68561fe97b9f15a083a0b42aa9955) testprojects/src/java/org/pantsbuild/testproject/bench/BUILD (2ec6296ffbc0fd29bd720c0b6d3a2c3e9e3a1113) testprojects/src/java/org/pantsbuild/testproject/bench/JmhBench.java (PRE-CREATION) tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py (6c087953baca41293055421fb4c243a7ea64d78f) tests/python/pants_test/backend/jvm/zinc/test_zinc_analysis.py (c2c1e5bee64f73d27b3a54c979d1f40195892fa4) tests/python/pants_test/backend/jvm/zinc/testdata/simple/README (PRE-CREATION) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.analysis (3fc5911d9ce20923697eceb5a2e337a0e6e99331) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.analysis (708cb6ec5d69cc7ab4972b440a21b2c16189c37d) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.filtered.analysis (6f0f8999d51ccb29afc38c3aca7340fa02ce4bde) View Diff

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 3, 2016, 5:50 p.m. Changes Add known issues to description Bugs: 3962 Repository: pants Description (updated)

This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data.

Known issues: With name hashing now as default, we will need to track transitive inheritances in pants, in another words, we get only direct ancestor from analysis, others ancestors need to be reconstructed. Which means unused-deps will see more false positives because of this. There is 10-20% overall slowness, mostly from rebasing since new analysis contains 5times more lines and 50% more bytes. Will address both issues in follow up reviews

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang. Updated Nov. 4, 2016, 5:10 p.m. Changes +stu

Bugs: 3962 Repository: pants Description (updated)

This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues:

With name hashing now as default, we will need to track transitive inheritances in pants, in another words, we get only direct ancestor from analysis, others ancestors need to be reconstructed. Which means unused-deps will see more false positives because of this.

There is 10-20% overall slowness, mostly from rebasing since new analysis contains 5times more lines and 50% more bytes. See https://rbcommons.com/s/twitter/r/4352/

Will address both issues in follow up reviews

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

Ship it!

Thanks Peiyu!

src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (Diff revision 1)
		42
		43	@property
		44	def zinc_file_manager(self):
		45	"""If false, the default file manager will be used instead of the zinc provided one.
		46	:rtype: bool
		47	"""
		48	return self.get_options().zinc_file_manager

Don't know how much time you have for this, but would really prefer that you add an "option sets" concept.
The target would specify string keys corresponding to option sets that are globally configured on the subsystem (with a default set of sets).

src/python/pants/backend/jvm/zinc/zinc_analysis_element_types.py (Diff revision 1)
		60	# self.internal_src_dep, self.external_dep,
		61	# self.internal_src_dep_pi, self.external_dep_pi,

xx

- Stu Hood

On November 4th, 2016, 5:10 p.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang. Updated Nov. 4, 2016, 5:10 p.m.

Bugs: 3962 Repository: pants Description This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 9, 2016, 7:18 p.m. Changes Address some review comment: remove stale comment Merge with master, now zinc-0.0.5 is published PR, travis Will follow up with another view for option sets before shipping this. Bugs: 3962, 4042 Repository: pants Description (updated)

This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues:

Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants Unreported dependencies from local anonymous classes https://github.com/sbt/zinc/issues/192 Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure. Will follow up the above issues Testing (updated) https://travis-ci.org/peiyuwang/pants/builds/174567118 Diffs (updated) 3rdparty/jvm/org/openjdk/jmh/BUILD (bf01fff38bc32c49c7c7534cf6cb6a2b1d6f7931) contrib/scalajs/src/python/pants/contrib/scalajs/targets/scala_js_target.py (afc5d2f94e37ae8e300f4f23d79fcec3ad42fa59) src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (25abc3fff4e00db7f66f21fbe1cb2c9e95bde95c) src/python/pants/backend/jvm/targets/jvm_target.py (9ee97e148246505544d10768d84641cd9a12563b) src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (49b15cf6c140fae23eac53f2898be98faa31edc2) src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (36bb6631adc442706806f9949152879675a9702f) src/python/pants/backend/jvm/tasks/jvm_dependency_usage.py (a4f55f4335c51eb6e8de5e695ab689f3fb8bf8ba) src/python/pants/backend/jvm/zinc/zinc_analysis.py (2c00fc93f79e0f7f4dc7a4ce2d765f0b53240839) src/python/pants/backend/jvm/zinc/zinc_analysis_element_types.py (93d622d4804675226abeee99bc945ec3d0ce0cb5) src/python/pants/backend/jvm/zinc/zinc_analysis_parser.py (cdc93910605486f6ae0b93bb485eba7a21611015) testprojects/src/java/org/pantsbuild/testproject/bench/BUILD (55640dce797c4f3bba10c72d84c3252b388decf5) testprojects/src/java/org/pantsbuild/testproject/bench/JmhBench.java (60e10df3589abf5178b791bc580055fee70c02b3) tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py (14a733bd7cab043a6f74b0f317fc961c2cafc204) tests/python/pants_test/backend/jvm/zinc/test_zinc_analysis.py (df507b641562e89cbfd1b308584278cf1a09cecd) tests/python/pants_test/backend/jvm/zinc/testdata/simple/README (5f4edb13142ca415ecba53cdb645aec647ef3633) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.analysis (63e991ec1475f94088a95db63db51a7ec4689ecf) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.analysis (580e91abd8684ad5c8b50651c8575a4dd3657735) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.filtered.analysis (5941aa3ed24e0be7b29b192e0569709f1b3a7d9c) View Diff Show Changes

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

On November 7th, 2016, 11:17 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (Diff revision 1)
		42
		43	@property
		44	def zinc_file_manager(self):
		45	"""If false, the default file manager will be used instead of the zinc provided one.
		46	:rtype: bool
		47	"""
		48	return self.get_options().zinc_file_manager

Don't know how much time you have for this, but would really prefer that you add an "option sets" concept.
The target would specify string keys corresponding to option sets that are globally configured on the subsystem (with a default set of sets).

Like to ship the python change and jvm change together, but will follow up right after shipping this.

On November 7th, 2016, 11:17 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/zinc/zinc_analysis_element_types.py (Diff revision 1)
		60	# self.internal_src_dep, self.external_dep,
		61	# self.internal_src_dep_pi, self.external_dep_pi,

xx

removed.

- Peiyu

On November 9th, 2016, 7:18 p.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 9, 2016, 7:18 p.m. Bugs: 3962, 4042 Repository: pants Description

This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues:

Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants Unreported dependencies from local anonymous classes https://github.com/sbt/zinc/issues/192 Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure.

Will follow up the above issues

Testing https://travis-ci.org/peiyuwang/pants/builds/174567118 Diffs

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

Ship it!

Ship It!

- Stu Hood

On November 9th, 2016, 7:18 p.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang. Updated Nov. 9, 2016, 7:18 p.m.

Bugs: 3962, 4042 Repository: pants Description This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064

This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues: Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants Unreported dependencies from local anonymous classes https://github.com/sbt/zinc/issues/192 Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure. Will follow up the above issues

Testing https://travis-ci.org/peiyuwang/pants/builds/174567118 Diffs

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 11, 2016, 5:40 a.m. Changes Multiple fixes discovered through travis, now we use the published zinc. Performance: absolute path in computing the zinc_cache_dir results cache miss everytime in IT for compiler-interface Performance: BaseCompileIT overrides --cache-write-to to a temp location, including bootstraped zinc jar. Shading a 35M jar is very expensive, ~45sec, and this cost adds to every single BaseCompileIT test. Switch to only override --cache-compile-write-to. parse_deps: properly load source to dependencies mapping, see ZincAnalysisTestSimple fix https://rbcommons.com/s/twitter/r/4342/diff/2-3/ Now travis is green (and probably faster compared to master due to the BaseCompileIT --cache-write-to)

Bugs: 3962, 4042 Repository: pants Description

This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064

This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues: Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants Unreported dependencies from local anonymous classes https://github.com/sbt/zinc/issues/192 Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure. Will follow up the above issues

Testing (updated) https://travis-ci.org/peiyuwang/pants/builds/174567118 https://travis-ci.org/pantsbuild/pants/builds/174984898 Diffs (updated)

3rdparty/jvm/org/openjdk/jmh/BUILD (bf01fff38bc32c49c7c7534cf6cb6a2b1d6f7931) contrib/scalajs/src/python/pants/contrib/scalajs/targets/scala_js_target.py (afc5d2f94e37ae8e300f4f23d79fcec3ad42fa59) src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (25abc3fff4e00db7f66f21fbe1cb2c9e95bde95c) src/python/pants/backend/jvm/targets/jvm_target.py (9ee97e148246505544d10768d84641cd9a12563b) src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (49b15cf6c140fae23eac53f2898be98faa31edc2)

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (fd08883e4ab4504ff47e1f30a41910911de05ba0) src/python/pants/backend/jvm/tasks/jvm_dependency_usage.py (a4f55f4335c51eb6e8de5e695ab689f3fb8bf8ba) src/python/pants/backend/jvm/zinc/zinc_analysis.py (2c00fc93f79e0f7f4dc7a4ce2d765f0b53240839) src/python/pants/backend/jvm/zinc/zinc_analysis_element_types.py (77baea667e09cbbe13b41d5fde2249bbd8065fe7) src/python/pants/backend/jvm/zinc/zinc_analysis_parser.py (15430d41bdea1769295227a103982f736e68dcae) testprojects/src/java/org/pantsbuild/testproject/bench/BUILD (55640dce797c4f3bba10c72d84c3252b388decf5) testprojects/src/java/org/pantsbuild/testproject/bench/JmhBench.java (60e10df3589abf5178b791bc580055fee70c02b3) tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (f14371a116a5b52029b6f592b2838c0b65080a7d)

tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py (14a733bd7cab043a6f74b0f317fc961c2cafc204) tests/python/pants_test/backend/jvm/zinc/test_zinc_analysis.py (df507b641562e89cbfd1b308584278cf1a09cecd) tests/python/pants_test/backend/jvm/zinc/testdata/simple/README (5f4edb13142ca415ecba53cdb645aec647ef3633) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.analysis (63e991ec1475f94088a95db63db51a7ec4689ecf) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.analysis (580e91abd8684ad5c8b50651c8575a4dd3657735) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.filtered.analysis (5941aa3ed24e0be7b29b192e0569709f1b3a7d9c)

View Diff Show Changes

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 3)
71	'--cache-write-to=[\'{}\']'.format(cacheurl),	71	'--cache-compile-write-to=[\'{}\']'.format(cacheurl),

write cache artifacts to temp except for bootstrap is better, i.e, blacklist vs whitelist.
follow up in https://rbcommons.com/s/twitter/r/4368/

- Peiyu Wang

On November 11th, 2016, 5:40 a.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang. Updated Nov. 11, 2016, 5:40 a.m.

Bugs: 3962, 4042

Repository: pants Description This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues: Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants Unreported dependencies from local anonymous classes https://github.com/sbt/zinc/issues/192 Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure. Will follow up the above issues Testing

https://travis-ci.org/peiyuwang/pants/builds/174567118 https://travis-ci.org/pantsbuild/pants/builds/174984898 Diffs

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

On November 7th, 2016, 11:17 p.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (Diff revision 1)
		42
		43	@property
		44	def zinc_file_manager(self):
		45	"""If false, the default file manager will be used instead of the zinc provided one.
		46	:rtype: bool
		47	"""
		48	return self.get_options().zinc_file_manager

Don't know how much time you have for this, but would really prefer that you add an "option sets" concept.
The target would specify string keys corresponding to option sets that are globally configured on the subsystem (with a default set of sets).

On November 9th, 2016, 7:20 p.m. UTC, Peiyu Wang wrote:

Like to ship the python change and jvm change together, but will follow up right after shipping this.

Followed up at https://rbcommons.com/s/twitter/r/4374/

- Peiyu

On November 11th, 2016, 5:40 a.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 11, 2016, 5:40 a.m. Bugs: 3962, 4042 Repository: pants Description

This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues:

[Stu Hood]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

Fix it, then Ship it!

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (Diff revisions 2 - 3)
313	hasher.update(self.tool_jar(tool))	313	hasher.update(os.path.relpath(self.tool_jar(tool), self.get_options().pants_workdir))

...hm. Won't this cause the artifact to not be shared across pants installs? Is the pants_workdir relative, or absolute?

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 3)
70	'--cache-write',	70	'--cache-write',

Why not make this more specific as well? --cache-compile-write

- Stu Hood

On November 11th, 2016, 5:40 a.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang. Updated Nov. 11, 2016, 5:40 a.m.

Bugs: 3962, 4042

Repository: pants Description This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues: Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants Unreported dependencies from local anonymous classes https://github.com/sbt/zinc/issues/192 Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure. Will follow up the above issues Testing

https://travis-ci.org/peiyuwang/pants/builds/174567118 https://travis-ci.org/pantsbuild/pants/builds/174984898 Diffs

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

On November 15th, 2016, 12:35 a.m. UTC, Stu Hood wrote:

src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (Diff revisions 2 - 3)
313	hasher.update(self.tool_jar(tool))	313	hasher.update(os.path.relpath(self.tool_jar(tool), self.get_options().pants_workdir))

...hm. Won't this cause the artifact to not be shared across pants installs? Is the pants_workdir relative, or absolute?

workdir is absolute, in integration test this will be a temp directory for each test, like --pants-workdir=/var/folders/z8/hfw3c_sj25b2t3wmnlfq3kjm0000gn/T/tmpBdSfsZ.pants.d
therefore the zinc artifact cached in one test is not reusable by another test.

On November 15th, 2016, 12:35 a.m. UTC, Stu Hood wrote:

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 3)
70	'--cache-write',	70	'--cache-write',

Why not make this more specific as well? --cache-compile-write

i think in test we would like each test to isolate their cache directories, not to share.
therefore whitelisting only those expensive ones is safer.
ps: https://rbcommons.com/s/twitter/r/4368 is a separte patch since it is really an independent bug.

- Peiyu

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 18, 2016, 4:09 a.m. Changes Merge with master to have the patch for zinc bootstrap slowness in IT https://rbcommons.com/s/twitter/r/4368/

Bugs: 3962, 4042 Repository: pants Description This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues: Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants Unreported dependencies from local anonymous classes https://github.com/sbt/zinc/issues/192 Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure. Will follow up the above issues Testing (updated)

https://travis-ci.org/peiyuwang/pants/builds/174567118 https://travis-ci.org/pantsbuild/pants/builds/174984898 https://travis-ci.org/pantsbuild/pants/builds/176920777 Diffs (updated)

3rdparty/jvm/org/openjdk/jmh/BUILD (bf01fff38bc32c49c7c7534cf6cb6a2b1d6f7931) contrib/scalajs/src/python/pants/contrib/scalajs/targets/scala_js_target.py (afc5d2f94e37ae8e300f4f23d79fcec3ad42fa59) src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (25abc3fff4e00db7f66f21fbe1cb2c9e95bde95c) src/python/pants/backend/jvm/targets/jvm_target.py (9ee97e148246505544d10768d84641cd9a12563b) src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (49b15cf6c140fae23eac53f2898be98faa31edc2) src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (fd08883e4ab4504ff47e1f30a41910911de05ba0) src/python/pants/backend/jvm/tasks/jvm_dependency_usage.py (a4f55f4335c51eb6e8de5e695ab689f3fb8bf8ba) src/python/pants/backend/jvm/zinc/zinc_analysis.py (2c00fc93f79e0f7f4dc7a4ce2d765f0b53240839) src/python/pants/backend/jvm/zinc/zinc_analysis_element_types.py (77baea667e09cbbe13b41d5fde2249bbd8065fe7)

src/python/pants/backend/jvm/zinc/zinc_analysis_parser.py (d565ddd6bb00e6ec1d277cf6d19d50073778b4fc)

testprojects/src/java/org/pantsbuild/testproject/bench/BUILD (55640dce797c4f3bba10c72d84c3252b388decf5) testprojects/src/java/org/pantsbuild/testproject/bench/JmhBench.java (60e10df3589abf5178b791bc580055fee70c02b3) tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py (14a733bd7cab043a6f74b0f317fc961c2cafc204)

tests/python/pants_test/backend/jvm/zinc/test_zinc_analysis.py (ec635187fc907036a628f430cfe778b698333009)

tests/python/pants_test/backend/jvm/zinc/testdata/simple/README (5f4edb13142ca415ecba53cdb645aec647ef3633) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.analysis (63e991ec1475f94088a95db63db51a7ec4689ecf) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.analysis (580e91abd8684ad5c8b50651c8575a4dd3657735) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.filtered.analysis (5941aa3ed24e0be7b29b192e0569709f1b3a7d9c)

View Diff Show Changes

[Peiyu Wang]

This is an automatically generated e-mail. To reply, visit: https://rbcommons.com/s/twitter/r/4342/

On November 15th, 2016, 12:35 a.m. UTC, Stu Hood wrote:

tests/python/pants_test/backend/jvm/tasks/jvm_compile/base_compile_integration_test.py (Diff revision 3)
70	'--cache-write',	70	'--cache-write',

Why not make this more specific as well? --cache-compile-write

On November 15th, 2016, 12:47 a.m. UTC, Peiyu Wang wrote:

i think in test we would like each test to isolate their cache directories, not to share.
therefore whitelisting only those expensive ones is safer.
ps: https://rbcommons.com/s/twitter/r/4368 is a separte patch since it is really an independent bug.

https://rbcommons.com/s/twitter/r/4368/ is merged, updated rb with master

- Peiyu

On November 18th, 2016, 4:09 a.m. UTC, Peiyu Wang wrote:

Review request for pants-reviews, Benjy Weinberger, Ity Kaul, Mateo Rodriguez, Nick Howard (Twitter), Stu Hood, Yi Cheng, and Eric Ayers. By Peiyu Wang.

Updated Nov. 18, 2016, 4:09 a.m.

Bugs: 3962, 4042 Repository: pants Description This review is based off Stu's unsubmitted previous effort: https://rbcommons.com/s/twitter/r/4064 This review depends on its jvm portion https://rbcommons.com/s/twitter/r/4340 It has everything from rb/4064: Deprecate the name-hashing flag: see buddy review. Update zinc parser for new analysis headers. Pass an explicit -cache-dir for zinc to compile the compiler-bridge into. Bump implementation version of zinc to account for the analysis format change. Don't iterate over source files while computing per-target deps. Plus a few other changes: A target flag zinc_file_manager to turn off zinc provided file manager, implementation is similar to fatal_warnings Add a jmh test for the new zinc_file_manager target flag. Fixed test_zinc_analysis, regenerated test data. Known issues: Unreported dependencies from indirect ancestors due to name hashing switch, will have to reconstruct in pants Unreported dependencies from local anonymous classes https://github.com/sbt/zinc/issues/192 Performance: incremental compile in some cases shows significant slowdowns (50%-80%), will collect more stats, maybe memory pressure. Will follow up the above issues Testing

https://travis-ci.org/peiyuwang/pants/builds/174567118 https://travis-ci.org/pantsbuild/pants/builds/174984898 https://travis-ci.org/pantsbuild/pants/builds/176920777 Diffs

3rdparty/jvm/org/openjdk/jmh/BUILD (bf01fff38bc32c49c7c7534cf6cb6a2b1d6f7931) contrib/scalajs/src/python/pants/contrib/scalajs/targets/scala_js_target.py (afc5d2f94e37ae8e300f4f23d79fcec3ad42fa59) src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py (25abc3fff4e00db7f66f21fbe1cb2c9e95bde95c) src/python/pants/backend/jvm/targets/jvm_target.py (9ee97e148246505544d10768d84641cd9a12563b) src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py (49b15cf6c140fae23eac53f2898be98faa31edc2) src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py (fd08883e4ab4504ff47e1f30a41910911de05ba0) src/python/pants/backend/jvm/tasks/jvm_dependency_usage.py (a4f55f4335c51eb6e8de5e695ab689f3fb8bf8ba) src/python/pants/backend/jvm/zinc/zinc_analysis.py (2c00fc93f79e0f7f4dc7a4ce2d765f0b53240839) src/python/pants/backend/jvm/zinc/zinc_analysis_element_types.py (77baea667e09cbbe13b41d5fde2249bbd8065fe7)

src/python/pants/backend/jvm/zinc/zinc_analysis_parser.py (d565ddd6bb00e6ec1d277cf6d19d50073778b4fc) testprojects/src/java/org/pantsbuild/testproject/bench/BUILD (55640dce797c4f3bba10c72d84c3252b388decf5) testprojects/src/java/org/pantsbuild/testproject/bench/JmhBench.java (60e10df3589abf5178b791bc580055fee70c02b3) tests/python/pants_test/backend/jvm/tasks/jvm_compile/java/test_zinc_compile_integration.py (14a733bd7cab043a6f74b0f317fc961c2cafc204) tests/python/pants_test/backend/jvm/zinc/test_zinc_analysis.py (ec635187fc907036a628f430cfe778b698333009)

tests/python/pants_test/backend/jvm/zinc/testdata/simple/README (5f4edb13142ca415ecba53cdb645aec647ef3633) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.analysis (63e991ec1475f94088a95db63db51a7ec4689ecf) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.analysis (580e91abd8684ad5c8b50651c8575a4dd3657735) tests/python/pants_test/backend/jvm/zinc/testdata/simple/simple.rebased.filtered.analysis (5941aa3ed24e0be7b29b192e0569709f1b3a7d9c)

View Diff