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_bzl_disallow_load_after_statement: Require load() be at the top of bzl files #5815

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

Comments

@c-parsons
Copy link
Contributor

commented Aug 8, 2018

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

This flag would enforce that bzl files have load() statements listed first.

@ittaiz

This comment has been minimized.

Copy link
Member

commented Aug 14, 2018

should the title change? It talks ab out BUILD files but the content seems to be about bzl files?
Also is this a constraint on the WORKSPACE file as well?

@c-parsons c-parsons changed the title Require load() be at the top of BUILD files Require load() be at the top of bzl files Aug 14, 2018

@c-parsons

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2018

Apologies, yes, this is about bzl files. I've changed the title.
This would not be a constraint on WORKSPACE files.

@laurentlb

This comment has been minimized.

Copy link
Member

commented Aug 20, 2018

We should do the same for BUILD files (we can track it in a separate issue). But not for WORKSPACE files, because they are evaluated in a relatively different way: previous statements affect the semantics of load.

@dslomov dslomov changed the title Require load() be at the top of bzl files ncompatible_bzl_disallow_load_after_statement: Require load() be at the top of bzl files 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.

@dslomov dslomov changed the title ncompatible_bzl_disallow_load_after_statement: Require load() be at the top of bzl files incompatible_bzl_disallow_load_after_statement: Require load() be at the top of bzl files Oct 31, 2018

@ittaiz

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

I think there may be a gap here- bzl files which are used only in the WORKSPACE file (directly and indirectly) need to be able to have load statement in the middle (like the WORKSPACE itself).
Use-case is the deps pattern of manual recursive workspaces

@laurentlb

This comment has been minimized.

Copy link
Member

commented Dec 12, 2018

cc @aehlig

@aehlig

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

cc @aehlig

As mentioned in a related discussion, I think restricting load statements to the beginning of a file is fine in the context of BUILD files (i.e., loaded directly or indirectly from a BUILD file). BUILD files are evaluated at a point of time where all the sources of files are already well defined.

In the context of WORKSPACE files, however, the situation is different as external repositories are still being defined (and hence new files to load from are being made available). In that context (i.e., when loaded directly or indirectly from the WORKSPACE file) I would suggest either allowing load statements everywhere, or at least support delaying the loading till the symbol is used for the first time. Currently, we are in the very unnatural situation that defining an external repository and then calling a Starlark function defined in one of its files is something that can be done top-level in the WORKSPACE file, but cannot be delegated to a macro.

@laurentlb

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

I agree the current situation feels unnatural, mainly because evaluation of WORKSPACE first splits the file. I think we don't split bzl files at all, even when they're loaded from a WORKSPACE file, right? So the main problem with incompatible_bzl_disallow_load_after_statement is that it prevents you from changing the semantics (but it doesn't affect current users)?

We can continue the discussion on the mailing-list, and see what we can do.

@ittaiz

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

@bazel-io bazel-io closed this in 3992d29 Mar 22, 2019

emusand added a commit to emusand/bazel that referenced this issue Apr 16, 2019

Enable --incompatible_bzl_disallow_load_after_statement by default
Fixes bazelbuild#5815

Tested: https://buildkite.com/bazel/bazelisk-plus-incompatible-flags/builds/49

RELNOTES: --incompatible_bzl_disallow_load_after_statement is enabled
PiperOrigin-RevId: 239854652
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.