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

Deprecate ZincCompile task in favor of RscCompile #8047

Merged
merged 15 commits into from Jul 27, 2019

Conversation

@ity
Copy link
Contributor

commented Jul 12, 2019

Problem

Deprecate ZincCompile task in favor of RscCompile which has a zinc-only option identical to what ZincCompile implements.

Solution

  • Deprecate use of ZincCompile
  • Set the removal version to 1.20.0.dev0 (at which point, RscCompile can continue to inherit from BaseZincCompile instead of ZincCompile).

Result

Fixes #7824.

@ity ity changed the title wip - deprecate ZincCompile task only (sans refactor) wip - deprecate ZincCompile task in favor of RscCompile Jul 12, 2019

@ity ity changed the title wip - deprecate ZincCompile task in favor of RscCompile deprecate ZincCompile task in favor of RscCompile Jul 12, 2019

@ity ity requested review from cosmicexplorer, illicitonion and stuhood Jul 12, 2019

@ity ity force-pushed the ity:ity/deprecate_zinc branch from 8d25c4d to 970e9b5 Jul 12, 2019

@stuhood
Copy link
Member

left a comment

Thanks @ity!

@@ -628,6 +629,10 @@ def process_info_file(cp_elem, info_file):
class ZincCompile(BaseZincCompile):
"""Compile Scala and Java code to classfiles using Zinc."""

deprecated_module(

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 16, 2019

Member

Should remove this: it will fire on every import.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 17, 2019

Member

(...I'm actually not sure about this now, because this concrete class should not be extended by anyone? If you're not seeing it trigger on import of this file, then let's leave it I guess. Sorry!)

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

Yea, it's still firing. Should remove or move it into the constructor as part of #8109.

Show resolved Hide resolved src/python/pants/backend/jvm/subsystems/jvm_platform.py
@@ -107,6 +107,9 @@ class RscCompile(ZincCompile, MirroredTargetOptionMixin):
_name = 'mixed' # noqa
compiler_name = 'rsc'

deprecated_options_scope = 'compile.zinc'

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 16, 2019

Member

Related to this: there is one more thing that we need to do.

As you've probably noticed: declaring a deprecated options scope like this won't also cause rsc to end up consuming the cache-compile-zinc options, because this task won't declare a dependency on that scope. It's very good that you went and fixed all the usages, because even if that substem actually existed, we'd want to fix the references.

But I think that we should ensure that that subsystem actually exists so that folks get a deprecation warning.

To do that, you can use "deprecated subsystem dependency":

def subsystem_dependencies(cls):
  ...
  dep = CacheSetup.scoped(
      Zinc,
      removal_version='1.19.0.dev0',
      removal_hint='Cache options for `zinc` should move to `rsc`.'
    )

... this will declare the subsystem, and will trigger a deprecation when it is used, but it won't actually cause you to use those options. I think that this is the best that we can do without a bunch of hacks, so I think it's worth going for.

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 16, 2019

Member

(this is the only blocker for landing this review, IIRC.)

This comment has been minimized.

Copy link
@ity

ity Jul 25, 2019

Author Contributor

implemented below:

@@ -56,6 +59,11 @@ class JvmCompile(CompilerOptionSetsMixin, NailgunTaskBase):

size_estimators = create_size_estimators()

class Compiler(enum.Enum):

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 16, 2019

Member

We're not using this in the option value for the compiler.

This comment has been minimized.

Copy link
@ity

ity Jul 25, 2019

Author Contributor

i think i missed this comment, will circle back

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

IMO, can just merge on green and then delete this later.

@ity ity force-pushed the ity:ity/deprecate_zinc branch 6 times, most recently from e39965e to 682da13 Jul 17, 2019

@ity ity force-pushed the ity:ity/deprecate_zinc branch 5 times, most recently from 1d98231 to 448750a Jul 24, 2019

@ity

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

@stuhood @cosmicexplorer - could you folks have another look? travis ci finally looks closer than ever to green! and I created #8109 for one of the failures.

@stuhood
Copy link
Member

left a comment

Thanks! Will merge when the last shard goes green.

@@ -56,6 +59,11 @@ class JvmCompile(CompilerOptionSetsMixin, NailgunTaskBase):

size_estimators = create_size_estimators()

class Compiler(enum.Enum):

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

IMO, can just merge on green and then delete this later.


if requested_compiler == self.Compiler.ZINC and self.compiler_name == self.Compiler.RSC:
# Issue a deprecation warning (above) and rewrite zinc to rsc, as zinc is being deprecated.
JvmPlatform.global_instance().get_options().compiler = RankedValue(0, self.compiler_name)

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

This has an ordering issue, but can fix in a followup.

@@ -110,8 +110,7 @@ def test_no_dependencies_between_scala_and_java_targets(self):
print(dependee_graph)
self.assertEqual(dedent("""
zinc[zinc-java](java/classpath:java_lib) <- {}
rsc(scala/classpath:scala_lib) <- {}
zinc[rsc-and-zinc](scala/classpath:scala_lib) <- {}""").strip(),
zinc[zinc-only](scala/classpath:scala_lib) <- {}""").strip(),

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

This probably represents a bug, but can fix in #8109. Probably need to set the workflow here.

@@ -43,5 +43,4 @@ def test_warning_filter(self):
},
})
self.assert_success(non_warning_run)
self.assertNotIn('DEPRECATED', non_warning_run.stderr_data)

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

Should loop back to fix this.

@@ -237,7 +237,6 @@ def test_non_recursive_quiet_no_output(self):
pants_run = self.run_pants(['-q', 'compile'])
self.assert_success(pants_run)
self.assertEqual('', pants_run.stdout_data)
self.assertEqual('\n', pants_run.stderr_data)

This comment has been minimized.

Copy link
@stuhood

stuhood Jul 25, 2019

Member

Should switch this to using another goal rather than removing this check.

@stuhood

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

I'm going to see if we can 1) land #8108 and, 2) get a full run of #8110 (and ideally land it) before landing this, because the last shard seems to be timing out determinstically.

@stuhood stuhood force-pushed the ity:ity/deprecate_zinc branch from 453f99d to b76c7f8 Jul 25, 2019

@stuhood stuhood force-pushed the ity:ity/deprecate_zinc branch from b76c7f8 to 8d03a48 Jul 27, 2019

@stuhood stuhood changed the title deprecate ZincCompile task in favor of RscCompile Deprecate ZincCompile task in favor of RscCompile Jul 27, 2019

@stuhood stuhood merged commit 96f6dd2 into pantsbuild:master Jul 27, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@benjyw

This comment has been minimized.

Copy link
Contributor

commented Aug 3, 2019

Reminder for the future, that we generally decided that deprecations should be on dev2 and not dev0... :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.