only filters labels according to parent packages for LanguageClass.C #47

ittaiz · 2019-11-13T08:04:48Z

This is to keep supporting CPP need while also supporting aggregate targets
More info:
https://bazelbuild.slack.com/archives/CM8JQCANN/p1566481600003600

Filter was deliberately added to fix an issue with C++ headers.
The original bug was that opening the BUILD file for foo/bar.h caused it to open baz/qux/BUILD because there’s a cc_library target in baz/qux that includes foo/bar.h in its headers. The source-to-target look up queries the index for the target during the last sync which compiled the source file. In this case, it was the cc_library target in baz/qux.
So the fix was to ensure that the BUILD file is always for the parent package (foo/bar).

OTOH what we have with aggregate targets is that Open Corresponding BUILD File action, Copy BUILD Target String, and some custom dependency lookups don't work (for AutoDep we copied part of the implementation of findBuildTarget to work around it).

Example for aggregate target:

a
    b
        BUILD.bazel (java_library with srcs pointing to filegroup from c & d)
        c
          BUILD.bazel (filegroup with glob)
          C.java
        d
          BUILD.bazel (filegroup with glob)
          D.java

I tried adding a test for this change but there are no tests for findBuildTarget at all let alone for the CPP fix. This made me question whether I want to dive into it. My current stand is to add it as is but I also considered covering existing behavior first. Problem is that it's likely a significant investment on this change and they might fix it differently at the end (see slack https://bazelbuild.slack.com/archives/CM8JQCANN/p1566576477000600?thread_ts=1566481600.003600&cid=CM8JQCANN https://bazelbuild.slack.com/archives/CM8JQCANN/p1566576507000800?thread_ts=1566481600.003600&cid=CM8JQCANN). Reason I'm not doing the suggested fix there is that it's much more complicated without fully diving into the code at SourceToTargetMapImpl (which I haven't done).

@ZachiNachshon @liucijus would appreciate your thoughts (more on the issues mentioned above than the really small code change)

This is to keep supporting CPP need while also supporting aggregate targets More info: https://bazelbuild.slack.com/archives/CM8JQCANN/p1566481600003600 Filter was deliberately added to fix an issue with C++ headers. The original bug was that opening the BUILD file for foo/bar.h caused it to open baz/qux/BUILD because there’s a cc_library target in baz/qux that includes foo/bar.h in its headers. The source-to-target look up queries the index for the target during the last sync which compiled the source file. In this case, it was the cc_library target in baz/qux. So the fix was to ensure that the BUILD file is always for the parent package (foo/bar)

liucijus · 2019-11-13T09:19:04Z

base/src/com/google/idea/blaze/base/actions/BuildFileUtils.java

+        .getTargetsToBuildForSourceFile(file).stream();
+
+    LanguageClass fileLanguage = LanguageClass.fromExtension(FileUtilRt.getExtension(file.getName()).toLowerCase());
+    final Stream<Label> maybeFilteredLabels = (fileLanguage == LanguageClass.C) ?


It would be a great improvement if the name (or more names) communicate what's going on here:

filter is applicable only for C

what that filter does for that language

I thought about extracting a private method to express intent but that is not the style of the code. OTOH if we're not going to upstream it then maybe it's not such a big deal.

Regarding what it does for that language- honestly I'm not sure it's easy for me to explain this in a comment let alone in a short name. Maybe a private method with a long comment with roughly the above quoted info?

If you plan to contribute to upstream, then most likely they will inline your methods. I would prefer using at least better name for vars. If this is not enough, please comment.

please review now

What triggered my attention in the first place was name maybeFilteredLabels. Maybe it makes sense to inline it now?

I inlined it. Not sure I like it that much because it makes me see even more how much I miss partial application :)
In any case please review

Do you still need .filter(l -> l.blazePackage().equals(packagePath)) after the call to filterCRelatedLabelsWhichBelongToOtherPackages?

Of course not, I’m an ass :) thanks!
Removed

refactoring without tests is not a good idea :)

:) we're good?

…t's needed

liucijus · 2019-11-14T07:52:05Z

LGTM

ittaiz · 2019-11-14T07:52:47Z

@ZachiNachshon @liucijus my original dilemmas were about adding this in and adding it with/without test.
Seeing the current state with upstream I think it's important to add it in and I'll add it without a test.
Just FYI about the conclusions I came to after thinking about this some more

ZachiNachshon · 2019-11-14T08:05:02Z

You are correct about adding at least a basic test to cover this feature so we could catch any possible future issues around it. If it's not possible at the moment, lets add it to our backlog and I'll add it.

ittaiz · 2019-11-14T09:27:44Z

I’ll do it in a separate PR since I want minimum noise here (hoping upstream will consider taking it)

…47) * only filters labels according to parent packages for LanguageClass.C This is to keep supporting CPP need while also supporting aggregate targets More info: https://bazelbuild.slack.com/archives/CM8JQCANN/p1566481600003600 Filter was deliberately added to fix an issue with C++ headers. The original bug was that opening the BUILD file for foo/bar.h caused it to open baz/qux/BUILD because there’s a cc_library target in baz/qux that includes foo/bar.h in its headers. The source-to-target look up queries the index for the target during the last sync which compiled the source file. In this case, it was the cc_library target in baz/qux. So the fix was to ensure that the BUILD file is always for the parent package (foo/bar)

liucijus reviewed Nov 13, 2019

View reviewed changes

ittaiz and others added 3 commits November 13, 2019 21:54

extract a private method with better name and add comment about why i…

c1ae986

…t's needed

inline

22eb3db

Remove filter

a5de013

ittaiz merged commit aa9895d into wix-master Nov 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

only filters labels according to parent packages for LanguageClass.C #47

only filters labels according to parent packages for LanguageClass.C #47

ittaiz commented Nov 13, 2019

liucijus Nov 13, 2019

ittaiz Nov 13, 2019

liucijus Nov 13, 2019

ittaiz Nov 13, 2019

liucijus Nov 13, 2019

ittaiz Nov 13, 2019

liucijus Nov 14, 2019

ittaiz Nov 14, 2019

liucijus Nov 14, 2019

ittaiz Nov 14, 2019

liucijus commented Nov 14, 2019

ittaiz commented Nov 14, 2019

ZachiNachshon commented Nov 14, 2019 •

edited

Loading

ittaiz commented Nov 14, 2019

only filters labels according to parent packages for LanguageClass.C #47

only filters labels according to parent packages for LanguageClass.C #47

Conversation

ittaiz commented Nov 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liucijus commented Nov 14, 2019

ittaiz commented Nov 14, 2019

ZachiNachshon commented Nov 14, 2019 • edited Loading

ittaiz commented Nov 14, 2019

ZachiNachshon commented Nov 14, 2019 •

edited

Loading