Rename sets.bzl to old_sets.bzl

[bttk]

No description provided.

[laurentlb]

@c-parsons Shall we use fail instead of print?

Also, are we going to keep the name new_sets in the long-term? I'd prefer to reuse the sets name. Users should just update their code when they migrate to the new version.

[bttk]

Do you want to assert the alpha state of skylib?

Maybe it is an opportunity to establish a process for making breaking changes to the API? I'm not saying that is has to go as far as, for example, Abseil does.

I think there should be 3 stages:

0. Old API works, new API is not ready yet.
1. Old API is deprecated, users are notified they can opt-in to the new API.
2. New API is the default, users can opt-out and use the old API.
   For example change the name sets.bzl to old_sets.bzl
   Is there a way to opt-out using a WORKSPACE rule?
3. Old API is removed.

• great to have: Tooling that aids migration is available
• must have: Users are pointed to the issue where problems with migration are tracked.

[c-parsons]

My initial thought is that we may be able to skip (1) in your proposal. I think it is a nice-to-have for users to be able to use old libraries, but I think we want to aggressively stress that users of skylib upgrade to the new functionality ASAP, as soon as they upgrade.

One reason we should strive to avoid branching is that it would be unfortunate, as a user, to depend on two libraries A and B, where A depends on the old version of the library, and the B depends on the new version -- there may be significant incompatibility there.

As for print vs. fail, and print as a deprecation warning, I don't believe we have universal consensus on this yet. print is a bit of a flaky option (as I believe it does not always show up, depending on the print options of the bazel user), and fail risks being too aggressive, circumventing phases (1) and (2) as you've highlighted.

Sorry, this become a longwinded monologue about our options. Here's my stance (but I'm certainly willing to discuss and debate this further):

• At this time, lets be aggressive about turning down deprecated call sites and updating them in place. lets make changes to sets.bzl and feel free to remove functions immediately. When a user updates to a new version of bazel_skylib, they will be forced to update their code immediately, by design.
• As skylib because more stable (say as bazel reaches 1.0 next year?), instead of removing functions outright, lets ensure that we leave this (deprecated) code active but renamed. Then users can upgrade to a new version of skylib, but retaining compatibility becomes somewhat easy.

[bttk]

Right, I haven't thought of dependencies using different skylib versions. I think that in (1) everything should still work.

For (2) I see there a way to use a dummy rule in WORKSPACE as an incompatible flag for the project and its dependencies (based on native.existing_rules()).

@c-parsons Are there currently any plans for stabilizing skylib API? I find it surprising, that the standard library for Bazel doesn't use a process similar to Bazel's backward compatibility

At some point a process for skylib should be established and the same tools could be used by skylib users for their API changes.

Let's say we have a lib/deprecation.bzl with:

• check_deprecation(name, default_value = False) - Used by the library. Returns True if the deprecated feature should stop working.
• override_deprecation(name, value = False) - Used by libraries dependencies (maybe only through a method exported by the library). A repository rule overriding the default value in check_deprecation

Maybe a more generic name could be used - lib/incompatible.bzl ?

With these methods, the process would be:

1. Lib makes the change behind check_deprecation("foo", default_value = False)
   At this point dependencies can try to override to see if they're ready for (2) and (3)
2. Lib changes to default_value = True. Dependencies have to override to keep using the old API.
3. check_deprecation is removed from the lib. Overrides no longer change anything.

[laurentlb]

@c-parsons Are there currently any plans for stabilizing skylib API? I find it surprising, that the standard library for Bazel doesn't use a process similar to Bazel's backward compatibility

Bazel's backward compatibility mechanism is pretty new and we're still figuring out the details. I agree that Skylib should be more stable, and we definitely have a more stable approach before Bazel 1.0.

That being said, this set.bzl file was not well-thought-out, and we have to fix that quickly. Keeping the old code under old_sets.bzl might help users who want to migrate later.

[c-parsons]

Let's say we have a lib/deprecation.bzl with:

check_deprecation(name, default_value = False) - Used by the library. Returns True if the deprecated feature should stop working.
override_deprecation(name, value = False) - Used by libraries dependencies (maybe only through a method exported by the library). A repository rule overriding the default value in check_deprecation

Sorry, I don't quite understand this proposal. Perhaps a somewhat full example would help my understanding? (Feel free to share a Google Doc, as inlining on a github issue might be difficult to share something rigorous)

I'll respond to your other comments, though:

Right, I haven't thought of dependencies using different skylib versions. I think that in (1) everything should still work.

To clarify, I'm concerned with two libraries depending on the same release version of skylib, but with different deprecation policies. We have to be aware of interop between the "deprecated" way and the "new" way. In contrast, we do not have this requirement for bazel's Starlark semantics, because we know that, in a single build, a user must choose the semantics (they need to specify a consistent flag across the build).

@c-parsons Are there currently any plans for stabilizing skylib API? I find it surprising, that the standard library for Bazel doesn't use a process similar to Bazel's backward compatibility

The policies may need be different because the requirements, ecosystems, and integration points are different. I've highlighted one difference above: that we don't need to worry about semantic interop in Bazel. Another is that bazel-skylib needs to try to keep up with the current Bazel version, whereas bazel doesn't have that requirement, and can thus set its own deprecation/feature pace.

(Concretely, if Bazel deprecates an API for removal, we must remove our dependence on it from bazel-skylib ASAP. It is not enough to move bazel-skylib's dependence on the API to a deprecated section: the next version of Bazel may very well make the deprecated bazel-skylib section "invalid Starlark" and thus unloadable at all)

[bttk]

@c-parsons Which parts of this change are you willing to approve?

• print deprecation warning
• rename sets to old_sets
  • reexport old_sets in sets
• by the way - add me to CONTRIBUTORS

[c-parsons]

Lets take a step back. I may be missing some overall context here -- why are we deprecating sets.bzl altogether, anyway? (Was there some discussion around sets.bzl that I missed?) I know that these functions are inefficient in that they flatten the sets, but they are occasionally useful, for example, in testing.

Proposed a CONTIBUTORS change in #72 , because I also need to be in CONTRIBUTORS :)

[thomasvl]

Lets take a step back. I may be missing some overall context here -- why are we deprecating sets.bzl altogether, anyway? (Was there some discussion around sets.bzl that I missed?)

Look up the PR that created new_sets.bzl. That should have some pointers to the past discussions and the problems with it.

[c-parsons]

Look up the PR that created new_sets.bzl. That should have some pointers to the past discussions and the problems with it.

Got it. I understand much better now, thanks.

I would opt for:

• rename sets to old_sets
• reexport old_sets in sets

No print-deprecation warning needed.

• Lets follow up by migrating upstream callers to use old_sets directly if they need the old behavior
• Then, lets follow up by stopping the reexport of old_sets, and instead only exporting new_sets in sets.

How does this sound?

[bttk]

@c-parsons Sounds good.

Is there a tool that helps with mass refactoring of github repos?

[c-parsons]

Alas, I know of no such tool to deal with cross-repo refactoring. Lets address some major ones manually. Other repositories can follow themselves, and aren't forced to until they would update to the next bazel-skylib release.

[bttk]

Can you merge this? I don't have write access.