Organize scalapb rules

[andyscott]

Description

Renames the public facing parts of the scalapb rules/targets to have the the scalapb prefix. Existing functionality is preserved by introducing deprecated forwarding method.

The diff looks rather large, but it was merely files rename combined with method level renames.

Use of the deprecated methods yields messages like the following:

DEBUG: /private/var/tmp/_bazel_andyscott/f340ee18f4d5da61a0cf1e5a51d4bb3b/external/io_bazel_rules_scala/private/console.bzl:26:5: 
rules_scala::warning::deprecation>>
  //scala_proto:scala_proto_toolchain.bzl scala_proto_toolchain is deprecated.
  Please use //scalapb:scalapb_toolchain.bzl scalapb_toolchain instead.
  deprecated: //scala_proto:scala_proto_toolchain.bzl scala_proto_toolchain
  supported : //scalapb:scalapb_toolchain.bzl scalapb_toolchain
<<rules_scala::warning::deprecation
DEBUG: /private/var/tmp/_bazel_andyscott/f340ee18f4d5da61a0cf1e5a51d4bb3b/external/io_bazel_rules_scala/private/console.bzl:26:5: 
rules_scala::warning::deprecation>>
  //scala_proto:scala_proto.bzl scalapb_proto_library is deprecated.
  Please use //scalapb:scalapb.bzl scalapb_proto_library instead.
  deprecated: //scala_proto:scala_proto.bzl scalapb_proto_library
  supported : //scalapb:scalapb.bzl scalapb_proto_library
<<rules_scala::warning::deprecation

This shouldn't have any impact on current usage other than outputting warnings like the above.

Motivation

Naming for files/rules in rules_scala is inconsistent and should be cleaned up.

More generally, I propose that all public functions/rules/targets should be named with the prefix of their path or owning file. You can see what I mean by looking at the names in this PR:

• function console_print_deprecation is in console.bzl
• rule scalapb_proto_library is in //scalapb:scalapb.bzl
• rule scalapb_repositories is in //scalapb:scalapb.bzl
• rule scalapb_toolchain is in //scalapb:scalapb_toolchain.bzl. Placing this in //scalapb:scalapb.bzl would also be acceptable.

[ittaiz]

I'm not sure this is the right direction...
For us it was important that the core of the rules remain proto centric and not around a specific generator (we've had several discussions about this over issues/PRs).
@simuons @viliusl @ignasl has anything changed? WDYT about this PR?

[andyscott]

I see your point about having tool agnostic names, although I don't care too much if the prefix is scala_proto or scalapb_ as long as naming is consistent. Currently the core API is named scalapb_proto_library but organized under scala_proto and alongside other APIs prefixed with scala_proto.

[johnynek]

My prior is to be negative on any change that is user visible. I think migrations and deprecations should be used VERY sparingly when the win can be clearly articulated. All software has warts, and I'm very suspect of breaking users (which can have a huge cost when multiplied out on all the users we don't see) to get marginally cleaner code.

So, that said, I would much rather we do any of this modularization in a way that doesn't break or deprecate anything. Frankly, I don't care much about the modularization: my editor (vim!) can find functions even if everything is in one file. Everything is in one repo, or one directory, or one github. It isn't obvious to me why multiple files are necessarily great, and I think the loss of the skylark privacy mechanism is real loss. I'm willing to be flexible since it seems this is of great importance to others and they are willing to trade the loss of the privacy mechanism for organizing things into files. By the way, an alternative would be block commented regions of the file:

#############
# scala protobuf support begins here....
#############
def scalapb_..

Anyway. Like I said, I'm willing to go along with the modularization, but I would prefer we take things in multiple independent steps:

1. modularization
2. renaming/deprecations

[andyscott]

Bazel is approaching 1.0, and it's only going to get more difficult to make user facing changes. The goal here, IMHO, is to have a high quality rule set for Scala. For me this includes clear intuitive organization of the code, as it affects users of the rules as well as developers of the rules.

Currently organization here isn't consistent. I like @ittaiz's notion of proto centric naming instead of implementation specific naming. We should apply this across the whole project:

• Core rules should be named in a functionality centric manner instead of a specific implementation manner.
• We try to organize APIs such that the prefixes are shared across files, paths, and starlark names. We already do this, mostly. A bit of cleanup would make it even better.

Practically speaking, this means that these rules remain scala_proto with a bit of cleanup:

• scalapb_proto_library becomes scala_proto_library. We can add a forwarding method without a deprecation notice.
• scalapb_toolchain optionally becomes scala_proto_scalapb_toolchain, again with forwarding methods for compatibility.

Additionally:

• twitter_scrooge related items are also inconsistent
  (twitter_scrooge exports scrooge_thrift. This should probably all be prefixed thrift_scala to be consistent with proto rules).

[johnynek]

Good to get to the meat of our disagreements....

I think we both value consistency, and we both value stability. I think what we have here is a conflict of how much stability to trade for consistency.

I would trade a little consistency for a lot of stability.

Next, I don't think users care how our code is organized. That is a feature for developers. Hurting users to help developers is generally a negative in my view, as a general principle, since we will have far more users than developers ideally (hopefully 2-3 orders of magnitude more, maybe 4-5?).

[andyscott]

First, one thing to be clear about: this PR shouldn't be merged. I was quickly turned to favor the scala_proto names.

Now, regarding the discussion:

There are only a handful of renames we need to make to have a well organized public facing API for rules_scala. I vote that we should make these changes now before Bazel hits a 1.0 release. We can keep aliases to maintain compatibility.

Going forward, we will also need a way to address breaking changes if we want to have any hope of merging functionality from https://github.com/higherkindness/rules_scala here. If we handle renames now, future changes should be isolated to rule attributes (mostly providers/toolchains) and configuration for rules_scala in WORKSPACE.

[andyscott]

I opened #821 to swing the change the other way: aligning on scala_proto for everything.

[SrodriguezO]

Hurting users to help developers is generally a negative in my view

I generally agree with this statement. However, I think having a clean, easy to navigate code base does help end users of OSS projects. Occasionally users will wish to understand what's happening under the hood, especially when things don't behave exactly as expected (which is surprisingly common). Additionally, a cleaner code base will make it easier for people to come up to speed and contribute, bringing more value to users.

Bazel is approaching 1.0, and it's only going to get more difficult to make user facing changes.

I think this is a very valid point--Bazel is still at a point where users can expect breaking changes to occur. We should definitely try to minimize these and their impact, but a deprecation warning at this stage in Bazel's development should not cause great concern among users imo. It'll be harder to tackle these issues in the future.

[johnynek]

I like #821 better. Should we close this one?

[andyscott]

Yeah totally 👍 on closing this one. I only left it open because of the discussion.