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_strict_argument_ordering #6611

Open
laurentlb opened this Issue Nov 6, 2018 · 13 comments

Comments

Projects
None yet
3 participants
@laurentlb
Copy link
Contributor

laurentlb commented Nov 6, 2018

Based on the discussion on bazelbuild/starlark#13, we're going to make argument ordering stricter. It also makes the semantics simpler to understand, and can simplify tooling.

All arguments should be in this order:

  • First, positional arguments
  • Then, keyword arguments
  • Then, an optional *arg
  • Finally, an optional **kwarg

e.g. f(a, b, c = 1, d = 2, *e, **f)

Migration: update buildifier to automatically fix the call sites.
It is also trivial to fix manually: change the order of the arguments.

- f(a, b, *c, d = 2)
+ f(a, b, d = 2, *c)

Expected timeline:

  • released with Bazel 0.21 (December 2018), off by default
  • enabled by default with Bazel 0.23 (February 2019)

bazel-io pushed a commit that referenced this issue Nov 6, 2018

Introduce --incompatible_strict_argument_ordering
This enables extra restrictions on arguments ordering.

#6611

RELNOTES: New incompatible flag --incompatible_strict_argument_ordering
PiperOrigin-RevId: 220330294

@laurentlb laurentlb changed the title incompatible_string_argument_ordering incompatible_strict_argument_ordering Nov 7, 2018

@laurentlb

This comment has been minimized.

Copy link
Contributor

laurentlb commented Jan 8, 2019

With this flag, only one downstream project fails: Gerrit.

ERROR: /var/lib/buildkite-agent/builds/bk-worker-ubuntu1604-java8-9b8n/bazel-downstream-projects/gerrit/tools/bzl/js.bzl:429:25: keyword argument is misplaced (keyword arguments must be before any *arg or **kwarg)

from https://gerrit.googlesource.com/gerrit/+/HEAD/tools/bzl/js.bzl#429
(cc @davido)

@davido

This comment has been minimized.

Copy link
Contributor

davido commented Jan 8, 2019

cc @dpursehouse

I fixed that and many other issues 2 months ago, but the review is staled:

https://gerrit-review.googlesource.com/c/gerrit/+/205653/1/tools/bzl/js.bzl#429

because new buildifier release doesn't work on macOS.

@dpursehouse

This comment has been minimized.

Copy link
Contributor

dpursehouse commented Jan 8, 2019

Here's the relevant PR on homebrew-core: Homebrew/homebrew-core#34815

@dpursehouse

This comment has been minimized.

Copy link
Contributor

dpursehouse commented Jan 8, 2019

Note that the PR is using 0.19.2.1. I have not tried it with the latest version 0.20.

@davido

This comment has been minimized.

Copy link
Contributor

davido commented Jan 8, 2019

@dpursehouse Thanks for the pointer.

What is the way forward? On Linux, I install buildifier from the source. Would it be an option for you as well on macOS to move forward with fixing Gerrit build tool chain, until homebrew-buildifier module is fixed?

We don't have a lot of time, because I would really like to avoid the situation when latest Bazel releases cannot build Gerrit master (and the tip of supported stable branches).

@dpursehouse

This comment has been minimized.

Copy link
Contributor

dpursehouse commented Jan 8, 2019

I wasn't able to build it from source either. I'll try again tomorrow with the latest release.

@laurentlb

This comment has been minimized.

Copy link
Contributor

laurentlb commented Jan 8, 2019

The PR looks great. Can it be merged?
Using Buildifier feels like a separate issue (it shouldn't block the fix).

@davido

This comment has been minimized.

Copy link
Contributor

davido commented Jan 8, 2019

I wasn't able to build it from source either.

My alias command to build buildifier is here:

alias buildbuildifier='bazel build --workspace_status_command=`pwd`/status.sh buildifier:buildifier'

Used bazel version is:

Build label: 0.22.0rc2

And most recent buildtool master just works: http://paste.openstack.org/show/740696

$ git describe --always 
bf564b4

$ buildbuildifier
[...]
external/com_google_protobuf: warning: directory does not exist.
Target //buildifier:buildifier up-to-date:
  bazel-bin/buildifier/linux_amd64_stripped/buildifier
INFO: Elapsed time: 125.923s, Critical Path: 18.72s
INFO: 281 processes: 280 linux-sandbox, 1 local.
INFO: Build completed successfully, 295 total actions

Test:

$ bazel-bin/buildifier/linux_amd64_stripped/buildifier --version
buildifier version: 0.20.0 
buildifier scm revision: bf564b4925ab5876a3f64d8b90fab7f769013d42

UPDATE:

I tried to use buildifier 0.20.0 and it seems to be broken in respect to --lint=fix mode. I swtiched back to that seems to work:

buildifier version: 0.19.2 
buildifier scm revision: d39e4d5c25111527369142f16cdb49aa67707313 
@dpursehouse

This comment has been minimized.

Copy link
Contributor

dpursehouse commented Jan 8, 2019

The PR looks great. Can it be merged?
Using Buildifier feels like a separate issue (it shouldn't block the fix).

Go ahead. We'll deal with the issue in Gerrit. Sorry for causing delays.

@dpursehouse

This comment has been minimized.

Copy link
Contributor

dpursehouse commented Jan 9, 2019

New PR on homebrew to update to 0.20.0 building it with bazel instead of go: Homebrew/homebrew-core#35816

@davido

This comment has been minimized.

Copy link
Contributor

davido commented Jan 9, 2019

@dpursehouse I see that buildifier PR to upgrade to 0.20.0 was merged in Homebrew, but unfortunately buildifier's --lint=fix options seems to be broken in 0.20.0 release. I filed this issue bazelbuild/buildtools#505 upstream and provided a reproducer, basically issue this command in Gerrit Code Review tree and compare the difference between buildifier 0.19.2 (works) and 0.20.0 (broken):

find . \( -name BUILD -o -name "*.bzl" \) -print xargs buildifier --lint=fix
@dpursehouse

This comment has been minimized.

Copy link
Contributor

dpursehouse commented Jan 9, 2019

@davido yes, I overlooked your comment before I went ahead and upgraded to 0.20.0

Do we then need to wait for a fixed version, or just use 0.20.0 without making use of the --lint feature?

@davido

This comment has been minimized.

Copy link
Contributor

davido commented Jan 9, 2019

Do we then need to wait for a fixed version, or just use 0.20.0 without making use of the --lint feature?

Let's wait and give Bazel and Buildtool folks react on my issue. I would really like to use buildifier's --lint=fix feature, because it automagically fixed so many issues in Gerrit built tool chain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment