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

incompatible_depset_union: Disallow union operator on depsets #5817

Closed
c-parsons opened this issue Aug 8, 2018 · 8 comments

Comments

@c-parsons
Copy link
Contributor

commented Aug 8, 2018

This is a tracking issue for offering a migration solution for
--incompatible_depset_union

Using +, | or .union on depsets is deprecated. Users should use the new
constructor instead (see https://docs.bazel.build/versions/master/skylark/depsets.html)

@c-parsons c-parsons self-assigned this Aug 8, 2018

@dslomov dslomov changed the title Disallow union operator on depsets incompatible_depset_union: Disallow union operator on depsets Oct 31, 2018

@dslomov

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

What is missing for migration: migration docs, length of migration window. After these are done, please add "migration-ready" label.

@davido

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

@laurentlb @c-parsons

It turns out that depset union feature is used very frequently in Gerrit Build tool chain. For example when running latest buildifier (0.20.0) in warn mode, we get this (on stable-2.14 branch in gerrit tree):

gerrit $ find . \( -name BUILD -o -name "*.bzl" \) -print | xargs buildifier --lint=warn
./tools/bzl/javadoc.bzl:23: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/javadoc.bzl:24: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/gwt.bzl:204: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/gwt.bzl:205: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/gwt.bzl:208: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/gwt.bzl:210: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/pkg_war.bzl:81: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/pkg_war.bzl:83: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/pkg_war.bzl:92: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/pkg_war.bzl:104: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/pkg_war.bzl:106: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/js.bzl:146: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/js.bzl:150: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/js.bzl:153: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/js.bzl:157: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/js.bzl:198: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/js.bzl:246: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/js.bzl:250: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/js.bzl:254: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/classpath.bzl:5: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/classpath.bzl:6: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)
./tools/bzl/classpath.bzl:8: depset-union: Depsets should be joined using the depset constructor. (https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#depset-union)

It also seems that at least in context of Polymer/Closure custom rules, that were written by @hanwen , it is not really trivial to avoid/re-write usage of depset-union feature.

I wonder if someone from Bazel team could help us to resolve those usages? Or give us more time and postpone deprecation of that feature? Or even re-consider and abandon the deprecation entirely?

Thanks!

@hanwen

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

AFAICT, this is just a syntactic change, where we can change

a.union(b)

to

a = depset(transitive = [a ,b])

@dslomov

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2019

Is it something we could easily implement in buildifier @c-parsons @vladmos ?

@laurentlb

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

Hanwen's solution is the simplest fix.

However, in many cases, the depsets are not properly used. For example, using union or + in a loop can lead to performance issues (https://docs.bazel.build/versions/master/skylark/performance.html#use-depsets).

If we update the code automatically, most users won't have the chance to look at the code and decide if it's a good thing to do.

For example,

d = depset()
for item in mylist:
  d += item.field

may be instead transformed to:

d = depset(transitive = [item.field for item in mylist])

xingao267 added a commit to xingao267/rules_docker that referenced this issue Jan 9, 2019

xingao267 added a commit to xingao267/rules_docker that referenced this issue Jan 9, 2019

xingao267 added a commit to xingao267/rules_docker that referenced this issue Jan 9, 2019

xingao267 added a commit to bazelbuild/rules_docker that referenced this issue Jan 11, 2019

Fix Bazel incompatible issues. (#648)
* Fix Bazel incompatible issues.

Some refs:
- bazelbuild/bazel#5817

bazel-io pushed a commit that referenced this issue Apr 25, 2019

Update tests to avoid depset union.
Progress towards #5817

RELNOTES: None.
PiperOrigin-RevId: 245242309

bazel-io pushed a commit that referenced this issue Apr 25, 2019

Update tests and source files to avoid depset union.
Progress towards #5817

RELNOTES: None.
PiperOrigin-RevId: 245265538

@bazel-io bazel-io closed this in 2cfa019 Apr 26, 2019

@c4urself

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Truly don't understand this change, if the workaround is a = depset(transitive=[a, b]) why not just make that the implementation for .union() or + behind the scenes instead of making people jump through hoops every few releases with these nonsensical changes. Python is the inspiration for Skylark and these changes are making everyone write/change more code for no reason (less Pythonic, more Javaesque)...

@laurentlb

This comment has been minimized.

Copy link
Member

commented May 29, 2019

Let me expand from my previous comment. Each call to depset has a cost. The goal is to make that cost more obvious for users. We did this change because the old behavior was the cause of lots of performance issues.

Old code (now forbidden):

all_deps = depset()
for dep in ctx.attr.deps:
  all_deps = all_deps + dep.files

Discouraged fix:

all_deps = depset()
for dep in ctx.attr.deps:
  all_deps = depset(transitive = [all_deps, dep.files])

Much better fix:

all_deps = depset(transitive = [dep.files for dep in ctx.attr.deps])

For a Python user, the old code might look fine and many people were writing this kind of code. If you blindly replace .union by its equivalent, the problem is more apparent. You are creating lots of depsets (it will be slow to iterate on those). The suggested fix is not longer and performs better.

So the design decision was made to prevent this tempting error.

@c4urself

This comment has been minimized.

Copy link
Contributor

commented May 29, 2019

Hmm, that last example makes a lot more sense to me -- will update to that, thanks for the quick reply!

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