Review Request 4388: Add the scala 2.12 platform

[Stu Hood]

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

Review request for pants-reviews, Benjy Weinberger, Mateo Rodriguez, Matt Olsen, and Yi Cheng. By Stu Hood. Bugs: 4068 Repository: pants Description Add a platform for 2.12 Drop version-specificity for scalastyle, since it only inspects the sources Float the version of scrooge-gen/scrooge-linter, because they do not mix classpaths with the things they are building/checking Testing https://travis-ci.org/pantsbuild/pants/builds/176902242 Diffs src/python/pants/backend/jvm/subsystems/scala_platform.py (78da286f9ca2539ba7a4f9952f66bf9ef96dbbe4) tests/python/pants_test/backend/jvm/subsystems/test_incomplete_custom_scala.py (3a8231d4c13cdc17ce6fd2e5ee43193667faf2c6) View Diff

[Benjy Weinberger]

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

I don't see anything relating to the scrooge changes you mentioned in the review description.
Also, why RB and not github pull request? :)

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 1)
19	# style_version - the version of org.scalastyle.scalastyle to use.	19	major_version_info = namedtuple('major_version_info', ['full_version'])

Do we need this namedtuple any more? Can we just use strings directly now?

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 1)
		31	# Because scalastyle inspects only the sources, it needn't match the platform version.

Is this always true? Wouldn't we need a minimum version of scalastyle that understands new language constructs if any?

- Benjy Weinberger

On November 18th, 2016, 1:52 a.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Mateo Rodriguez, Matt Olsen, and Yi Cheng. By Stu Hood.

Updated Nov. 18, 2016, 1:52 a.m.

[Stu Hood]

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

On November 18th, 2016, 4:54 a.m. UTC, Benjy Weinberger wrote:

I don't see anything relating to the scrooge changes you mentioned in the review description.
Also, why RB and not github pull request? :)

Ah, thanks. Updating now.

Also, why RB and not github pull request? :)

Indeed! Was planning to get that kicked off tomorrow. Sorry!

- Stu

[Stu Hood]

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

Review request for pants-reviews, Benjy Weinberger, Mateo Rodriguez, Matt Olsen, and Yi Cheng. By Stu Hood.

Updated Nov. 18, 2016, 5:21 a.m.

Bugs: 4068 Repository: pants Description Add a platform for 2.12 Drop version-specificity for scalastyle, since it only inspects the sources Float the version of scrooge-gen/scrooge-linter, because they do not mix classpaths with the things they are building/checking Testing https://travis-ci.org/pantsbuild/pants/builds/176902242

Diffs (updated) BUILD.tools (e018512fa81dd535a93d555fb066223bc34dc759) src/python/pants/backend/jvm/subsystems/scala_platform.py (78da286f9ca2539ba7a4f9952f66bf9ef96dbbe4) tests/python/pants_test/backend/jvm/subsystems/test_incomplete_custom_scala.py (3a8231d4c13cdc17ce6fd2e5ee43193667faf2c6) View Diff Show Changes

[Stu Hood]

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

On November 18th, 2016, 4:54 a.m. UTC, Benjy Weinberger wrote:

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 1)
19	# style_version - the version of org.scalastyle.scalastyle to use.	19	major_version_info = namedtuple('major_version_info', ['full_version'])

Do we need this namedtuple any more? Can we just use strings directly now?

Yes, probably. But I wasn't sure whether additional tool versions would be re-introduced at some point, so removing seemed premature.

On November 18th, 2016, 4:54 a.m. UTC, Benjy Weinberger wrote:

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 1)
		31	# Because scalastyle inspects only the sources, it needn't match the platform version.

Is this always true? Wouldn't we need a minimum version of scalastyle that understands new language constructs if any?

Yes, it is possible that new syntax (there hasn't been any in a long while) will require a new scalastyle version. But it is definitely not linked to the platform version currently, and it's not clear which platforms a new version will be available for... so IMO it's not worth attempting to guess the future there.

- Stu

On November 18th, 2016, 5:21 a.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Mateo Rodriguez, Matt Olsen, and Yi Cheng. By Stu Hood.

Updated Nov. 18, 2016, 5:21 a.m.

Bugs: 4068 Repository: pants Description Add a platform for 2.12 Drop version-specificity for scalastyle, since it only inspects the sources Float the version of scrooge-gen/scrooge-linter, because they do not mix classpaths with the things they are building/checking Testing https://travis-ci.org/pantsbuild/pants/builds/176902242 Diffs

BUILD.tools (e018512fa81dd535a93d555fb066223bc34dc759) src/python/pants/backend/jvm/subsystems/scala_platform.py (78da286f9ca2539ba7a4f9952f66bf9ef96dbbe4) tests/python/pants_test/backend/jvm/subsystems/test_incomplete_custom_scala.py (3a8231d4c13cdc17ce6fd2e5ee43193667faf2c6) View Diff

[Benjy Weinberger]

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

On November 18th, 2016, 4:54 a.m. UTC, Benjy Weinberger wrote:

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 1)
19	# style_version - the version of org.scalastyle.scalastyle to use.	19	major_version_info = namedtuple('major_version_info', ['full_version'])

Do we need this namedtuple any more? Can we just use strings directly now?

On November 18th, 2016, 5:24 a.m. UTC, Stu Hood wrote:

Yes, probably. But I wasn't sure whether additional tool versions would be re-introduced at some point, so removing seemed premature.

Fair enough.

On November 18th, 2016, 4:54 a.m. UTC, Benjy Weinberger wrote:

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 1)
		31	# Because scalastyle inspects only the sources, it needn't match the platform version.

Is this always true? Wouldn't we need a minimum version of scalastyle that understands new language constructs if any?

On November 18th, 2016, 5:24 a.m. UTC, Stu Hood wrote:

Yes, it is possible that new syntax (there hasn't been any in a long while) will require a new scalastyle version. But it is definitely not linked to the platform version currently, and it's not clear which platforms a new version will be available for... so IMO it's not worth attempting to guess the future there.

Right, so in that case we'd have to model language versions, which are distinct from runtime versions. That's technically true of Java, although they always release the two in lockstep in practice.

- Benjy

On November 18th, 2016, 5:21 a.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Mateo Rodriguez, Matt Olsen, and Yi Cheng. By Stu Hood.

Updated Nov. 18, 2016, 5:21 a.m.

Bugs: 4068 Repository: pants Description Add a platform for 2.12 Drop version-specificity for scalastyle, since it only inspects the sources Float the version of scrooge-gen/scrooge-linter, because they do not mix classpaths with the things they are building/checking Testing https://travis-ci.org/pantsbuild/pants/builds/176902242 Diffs

BUILD.tools (e018512fa81dd535a93d555fb066223bc34dc759) src/python/pants/backend/jvm/subsystems/scala_platform.py (78da286f9ca2539ba7a4f9952f66bf9ef96dbbe4) tests/python/pants_test/backend/jvm/subsystems/test_incomplete_custom_scala.py (3a8231d4c13cdc17ce6fd2e5ee43193667faf2c6) View Diff

[Benjy Weinberger]

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

Ship it!

Ship It!

- Benjy Weinberger

On November 18th, 2016, 5:21 a.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Mateo Rodriguez, Matt Olsen, and Yi Cheng. By Stu Hood. Updated Nov. 18, 2016, 5:21 a.m. Bugs: 4068 Repository: pants Description Add a platform for 2.12 Drop version-specificity for scalastyle, since it only inspects the sources Float the version of scrooge-gen/scrooge-linter, because they do not mix classpaths with the things they are building/checking Testing https://travis-ci.org/pantsbuild/pants/builds/176902242 Diffs

[Yi Cheng]

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

Ship it!

Ship It!

- Yi Cheng

On November 18th, 2016, 5:21 a.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Mateo Rodriguez, Matt Olsen, and Yi Cheng. By Stu Hood. Updated Nov. 18, 2016, 5:21 a.m. Bugs: 4068 Repository: pants Description Add a platform for 2.12 Drop version-specificity for scalastyle, since it only inspects the sources Float the version of scrooge-gen/scrooge-linter, because they do not mix classpaths with the things they are building/checking Testing https://travis-ci.org/pantsbuild/pants/builds/176902242 Diffs

[Mateo Rodriguez]

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

Ship it!

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 2)
78	rev = scala_build_info['2.10'].full_version	76	rev = scala_build_info[version].full_version

This seems like the right thing to ship, but it does hold the possibility of silently changing the jline version people are using. Seems like an unavoidable consequence of having hardcoded them in the first place, though
I think it is okay to assume that only a pants-expert is likely to be using with_jline and therefore would be able find this patch and understand it as a correctness improvement.

- Mateo Rodriguez

On November 17th, 2016, 11:21 p.m. CST, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Mateo Rodriguez, Matt Olsen, and Yi Cheng. By Stu Hood.

Updated Nov. 17, 2016, 11:21 p.m.

Bugs: 4068 Repository: pants Description Add a platform for 2.12 Drop version-specificity for scalastyle, since it only inspects the sources Float the version of scrooge-gen/scrooge-linter, because they do not mix classpaths with the things they are building/checking Testing https://travis-ci.org/pantsbuild/pants/builds/176902242 Diffs

[Stu Hood]

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

On November 18th, 2016, 4:25 p.m. UTC, Mateo Rodriguez wrote:

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 2)
78	rev = scala_build_info['2.10'].full_version	76	rev = scala_build_info[version].full_version

This seems like the right thing to ship, but it does hold the possibility of silently changing the jline version people are using. Seems like an unavoidable consequence of having hardcoded them in the first place, though
I think it is okay to assume that only a pants-expert is likely to be using with_jline and therefore would be able find this patch and understand it as a correctness improvement.

@mateo: The caller only ever passes 2.10 with with_jline, which is why this worked before.

- Stu

On November 18th, 2016, 5:21 a.m. UTC, Stu Hood wrote:

Review request for pants-reviews, Benjy Weinberger, Mateo Rodriguez, Matt Olsen, and Yi Cheng. By Stu Hood.

Updated Nov. 18, 2016, 5:21 a.m. Bugs: 4068

[Mateo Rodriguez]

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

On November 18th, 2016, 10:25 a.m. CST, Mateo Rodriguez wrote:

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 2)
78	rev = scala_build_info['2.10'].full_version	76	rev = scala_build_info[version].full_version

This seems like the right thing to ship, but it does hold the possibility of silently changing the jline version people are using. Seems like an unavoidable consequence of having hardcoded them in the first place, though
I think it is okay to assume that only a pants-expert is likely to be using with_jline and therefore would be able find this patch and understand it as a correctness improvement.

On November 18th, 2016, 10:32 a.m. CST, Stu Hood wrote:

@mateo: The caller only ever passes 2.10 with with_jline, which is why this worked before.

well, in Pants master. I override the repl and call this function outside of Pants code.

- Mateo

[Mateo Rodriguez]

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

On November 18th, 2016, 10:25 a.m. CST, Mateo Rodriguez wrote:

src/python/pants/backend/jvm/subsystems/scala_platform.py (Diff revision 2)
78	rev = scala_build_info['2.10'].full_version	76	rev = scala_build_info[version].full_version

This seems like the right thing to ship, but it does hold the possibility of silently changing the jline version people are using. Seems like an unavoidable consequence of having hardcoded them in the first place, though
I think it is okay to assume that only a pants-expert is likely to be using with_jline and therefore would be able find this patch and understand it as a correctness improvement.

On November 18th, 2016, 10:32 a.m. CST, Stu Hood wrote:

@mateo: The caller only ever passes 2.10 with with_jline, which is why this worked before.

On November 18th, 2016, 12:55 p.m. CST, Mateo Rodriguez wrote:

well, in Pants master. I override the repl and call this function outside of Pants code.

ha, oops. Lies. This is an internal method, so I had to reimplemented it internally. As you can probably tell, I do not add jline to our custom repl. Apologies, carry on.

- Mateo