Using Labels to Reference Directories

48 views
Skip to first unread message

Laurent Le Brun

unread,
Oct 19, 2018, 12:50:24 PM10/19/18
to baze...@googlegroups.com
The documentation mentions something about directory labels: https://docs.bazel.build/versions/master/build-ref.html#label_directory

The documentation says that "When you specify a directory, the build system will perform a rebuild only when the directory itself changes (due to addition or deletion of files), but won't be able to detect edits to individual files as those changes do not affect the enclosing directory."

However, I don't get any rebuild when I add or remove files, when I try the following BUILD file. I actually need to run "bazel clean" in order to get an updated output:

$ cat test/BUILD
genrule(
    name = "foo",
    srcs = ["sub/."],
    outs = ["out"],
    cmd = "ls -l test/sub/ | tee $@",
)

What is the status of this (mis)feature? 
Shall we just forbid directory labels?

--
Laurent

Tobias Werth

unread,
Oct 19, 2018, 12:54:18 PM10/19/18
to Laurent Le Brun, Ulf Adams, baze...@googlegroups.com

--
You received this message because you are subscribed to the Google Groups "bazel-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+...@googlegroups.com.
To post to this group, send email to baze...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/CAFtxR%2Bkg3SNXw1iEfUOz-yteD%3Detsc%2BAKiCPO7-BowCHVhQxtg%40mail.gmail.com.
For more options, visit https://groups.google.com/d/optout.

Ulf Adams

unread,
Oct 23, 2018, 7:06:14 AM10/23/18
to Tobias Werth, Laurent Le Brun, baze...@googlegroups.com
I have a pending patch that adds a flag to improve the behavior such that directories are tracked correctly for incremental builds:

The intend is to enable the new behavior by default and remove the flag and old code paths. This is mostly transparent for users - no migration needed - but there's some risk around performance and error handling, therefore the flag, and it's a startup flag since it requires a Skyframe cache drop any time the value is switched because it affects the Skyframe dependencies.


We can't forbid directory labels:
1. There are customers (both inside and outside of Google) depending on the ability to reference files without making them labels, since labels have a stricter syntax than files. While we've made some progress on loosening the restrictions, Bazel still can't handle general UTF-8.
2. They're a significant performance win as long as glob is eagerly evaluated during loading. I once floated the idea that maybe glob should be lazy, but that's also very hard to pull off since the result is available to macros (wide usage), and the alternative proposal of adding non-lazy glob or of adding lazy glob and migrating most users was also not well received (confusing API; requiring users to explicitly tell Bazel something that 'only' has performance implications).

Rock, meet hard place.

My conclusion was that we should make label directories 'work properly', and then enforce that such labels have a trailing '/' so that they can be easily identified at sight in the BUILD file (caveat: they can be returned from glob, although only if 'allow_directories=1' is set in the glob call, IIRC).

Lukács T. Berki

unread,
Oct 23, 2018, 7:58:16 AM10/23/18
to Ulf Adams, Tobias Werth, Laurent Le Brun, baze...@googlegroups.com
As much as I'd like to have proper handling of input directories, there are a few question marks in my head about this change:
  • Does it stop globbing at package boundaries? If not, what's the story about multiple --package_path entries? If yes, how does that work given that the naive "symlink the package directory into the execroot" approach result in the files of the subpackage being there, too?
  • Can this be rolled out with a non-startup flag so that it's more in line with the --incompatible_* flag policy? The usual approach is to put these flags into SkylarkSemantics, then access it through PrecomputedValue.SKYLARK_SEMANTICS. You then get proper Skyframe invalidation and it looks like the action cache should be okay, too.
  • The trailing-slash rule is not enforced in this change. Until it is, let's not roll this change out?



To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+unsubscribe@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "bazel-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+unsubscribe@googlegroups.com.

To post to this group, send email to baze...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/bazel-dev/CALS-RZKHbXvDfW3PkLeKSahfE_G8x0EJVZU7NKasTJvmacKjYQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.



--
Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

Lukács T. Berki

unread,
Oct 23, 2018, 8:01:16 AM10/23/18
to Ulf Adams, Tobias Werth, Laurent Le Brun, baze...@googlegroups.com, Dmitry Lomov
Do we want to consider this a nice to have for Bazel 1.0? Since it's a correctness issue, one could even argue to make this a blocker.

Ulf Adams

unread,
Oct 23, 2018, 11:24:23 AM10/23/18
to Lukacs Berki, Tobias Werth, Laurent Le Brun, baze...@googlegroups.com, Dmitry Lomov
On Tue, Oct 23, 2018 at 2:01 PM Lukács T. Berki <lbe...@google.com> wrote:
Do we want to consider this a nice to have for Bazel 1.0? Since it's a correctness issue, one could even argue to make this a blocker.

IMO, all known incremental correctness bugs should be blockers for 1.0. There are also possible races with concurrent modifications to the workspace that I wouldn't consider correctness bugs, although they're certainly problematic for user happiness.
 

On Tue, Oct 23, 2018 at 1:57 PM, Lukács T. Berki <lbe...@google.com> wrote:
As much as I'd like to have proper handling of input directories, there are a few question marks in my head about this change:
  • Does it stop globbing at package boundaries? If not, what's the story about multiple --package_path entries?
I think there's a discussion of that on the CL. If not, please start one there.
  • If yes, how does that work given that the naive "symlink the package directory into the execroot" approach result in the files of the subpackage being there, too?
I think this also belongs on the CL.
 
  • Can this be rolled out with a non-startup flag so that it's more in line with the --incompatible_* flag policy? The usual approach is to put these flags into SkylarkSemantics, then access it through PrecomputedValue.SKYLARK_SEMANTICS. You then get proper Skyframe invalidation and it looks like the action cache should be okay, too.

Conceptually, I don't believe that this should be an incompatible_ flag, according to the policy. I specifically asked about this on the doc, and I think a criteria of 'does not require user migration' is a reasonable bar for not marking a flag as 'incompatible'. I'm certainly not opposed to having infrastructure to avoid having these as startup options, but it doesn't really belong into SkylarkSemantics.
 
  • The trailing-slash rule is not enforced in this change. Until it is, let's not roll this change out?
I think that's completely orthogonal to the change here. I'm trying to put this in to fix the underlying serious correctness bug, the other is just a nice-to-have.
 



To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+...@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "bazel-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+...@googlegroups.com.

To post to this group, send email to baze...@googlegroups.com.
--
Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891

Lukács T. Berki

unread,
Oct 26, 2018, 11:07:05 AM10/26/18
to Ulf Adams, Tobias Werth, Laurent Le Brun, baze...@googlegroups.com, Dmitry Lomov
On Tue, Oct 23, 2018 at 5:24 PM, Ulf Adams <ulf...@google.com> wrote:
On Tue, Oct 23, 2018 at 2:01 PM Lukács T. Berki <lbe...@google.com> wrote:
Do we want to consider this a nice to have for Bazel 1.0? Since it's a correctness issue, one could even argue to make this a blocker.

IMO, all known incremental correctness bugs should be blockers for 1.0. There are also possible races with concurrent modifications to the workspace that I wouldn't consider correctness bugs, although they're certainly problematic for user happiness.
 

On Tue, Oct 23, 2018 at 1:57 PM, Lukács T. Berki <lbe...@google.com> wrote:
As much as I'd like to have proper handling of input directories, there are a few question marks in my head about this change:
  • Does it stop globbing at package boundaries? If not, what's the story about multiple --package_path entries?
I think there's a discussion of that on the CL. If not, please start one there.
Will do.
 
  • If yes, how does that work given that the naive "symlink the package directory into the execroot" approach result in the files of the subpackage being there, too?
I think this also belongs on the CL.
Will do.
 
 
  • Can this be rolled out with a non-startup flag so that it's more in line with the --incompatible_* flag policy? The usual approach is to put these flags into SkylarkSemantics, then access it through PrecomputedValue.SKYLARK_SEMANTICS. You then get proper Skyframe invalidation and it looks like the action cache should be okay, too.

Conceptually, I don't believe that this should be an incompatible_ flag, according to the policy. I specifically asked about this on the doc, and I think a criteria of 'does not require user migration' is a reasonable bar for not marking a flag as 'incompatible'. I'm certainly not opposed to having infrastructure to avoid having these as startup options, but it doesn't really belong into SkylarkSemantics.
If you want to roll this out without a flag, feel free to do so. But if you have a flag, let's make it a command flag instead of a startup one.
 
 
  • The trailing-slash rule is not enforced in this change. Until it is, let's not roll this change out?
I think that's completely orthogonal to the change here. I'm trying to put this in to fix the underlying serious correctness bug, the other is just a nice-to-have.
Sounds like a plan.
 
 



To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+unsubscribe@googlegroups.com.

--
You received this message because you are subscribed to the Google Groups "bazel-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to bazel-dev+unsubscribe@googlegroups.com.

To post to this group, send email to baze...@googlegroups.com.
--
Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891



--
Lukács T. Berki | Software Engineer | lbe...@google.com | 

Google Germany GmbH | Erika-Mann-Str. 33  | 80636 München | Germany | Geschäftsführer: Paul Manicle, Halimah DeLaine Prado | Registergericht und -nummer: Hamburg, HRB 86891
Reply all
Reply to author
Forward
0 new messages