search

bazelbuild / bazel-skylib

Common useful functions and rules for Bazel
https://bazel.build/
Apache License 2.0
388 stars 178 forks source link

Deprecate and remove lib.bzl #82

Open Capstan opened 5 years ago

Capstan commented 5 years ago

This process was started in commit 1099dd2 by @thomasvl .

  1. Use a print() warning.
  2. Wait one or two releases. ← we are here.
  3. Use a fail() error.
  4. Remove lib.bzl
manekinekko commented 5 years ago

Hi, I got a quick question about this deprecation. If I got this right:

BEFORE: load("@bazel_skylib//:lib.bzl", "paths") AFTER: load("@bazel_skylib//lib:paths.bzl", "paths")

Right?

Capstan commented 5 years ago

Correct. Or if you want to be pedantic about not re-exporting:

load("@bazel_skylib//lib:paths.bzl", _paths="paths")

manekinekko commented 5 years ago

So, in theory, this code shouldn't trigger the deprecation warnings? Right?

Because we are getting these warning in https://github.com/angular/angular/issues/27603

Capstan commented 5 years ago

Correct. Unfortunately, you'll still get the warning if any starlark in your transitive closure of loads loads lib.bzl, including in the WORKSPACE file, IIUC.

manekinekko commented 5 years ago

Gotcha. Thanks for the feedback.

laurentlb commented 5 years ago

In my experience, the print warning was not useful. Users should deal with it when they upgrade (so it should be an error). Some people didn't, because it's a warning and maybe they didn't see it. As a result, anyone building their code gets a notification in the console. It's quite noisy and it encourages everyone to ignore the messages. For most people seeing the message, it's not actionable, because it's not their code.

So let's replace it with fail now.