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

Fix common prefix for instrumentation filter #12607

Closed

Conversation

limdor
Copy link
Contributor

@limdor limdor commented Dec 2, 2020

In case that two path share a prefix, the instrumentation filter is
not computed proper.
#10949
This PR takes as a base the one that was created by @caiyulun and
adds some tests.
Original PR: #10949

In case that two path share a prefix, the instrumentation filter is
not computed proper.
bazelbuild#10949
This PR takes as a base the one that was created by @caiyulun and
adds some tests.
Original PR: bazelbuild#10949
listOfTargets.add(getTarget("//foo:t"));
listOfTargets.add(getTarget("//foobar:t"));
Collection<Target> targets = Collections.unmodifiableCollection(listOfTargets);
String expectedFilter = "^//foo[/:],^//foobar[/:]";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If prefered, I could split the PR in two commits to see more clear how the test would change with the changes.
Before the this PR the expected Filter would have been just ^//foo[/:]

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. We squash the PR into a single commit anyway, so there's limited value in splitting everything up incrementally in this case.

Copy link
Member

@c-mita c-mita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

listOfTargets.add(getTarget("//foo:t"));
listOfTargets.add(getTarget("//foobar:t"));
Collection<Target> targets = Collections.unmodifiableCollection(listOfTargets);
String expectedFilter = "^//foo[/:],^//foobar[/:]";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. We squash the PR into a single commit anyway, so there's limited value in splitting everything up incrementally in this case.

@limdor
Copy link
Contributor Author

limdor commented Dec 3, 2020

@c-mita some checks are failing but looks more like an http error connection.

@jin jin added the team-Rules-Server Issues for serverside rules included with Bazel label Dec 4, 2020
@philwo
Copy link
Member

philwo commented Dec 7, 2020

@c-mita and me are merging this right now and I'll make sure to get it into Bazel 4.0.0rc3! :)

@bazel-io bazel-io closed this in a9419f3 Dec 7, 2020
philwo pushed a commit that referenced this pull request Dec 7, 2020
In case that two path share a prefix, the instrumentation filter is
not computed proper.
#10949
This PR takes as a base the one that was created by @caiyulun and
adds some tests.
Original PR: #10949

Closes #12607.

PiperOrigin-RevId: 346097533
philwo pushed a commit that referenced this pull request Dec 9, 2020
In case that two path share a prefix, the instrumentation filter is
not computed proper.
#10949
This PR takes as a base the one that was created by @caiyulun and
adds some tests.
Original PR: #10949

Closes #12607.

PiperOrigin-RevId: 346097533
philwo pushed a commit that referenced this pull request Dec 10, 2020
In case that two path share a prefix, the instrumentation filter is
not computed proper.
#10949
This PR takes as a base the one that was created by @caiyulun and
adds some tests.
Original PR: #10949

Closes #12607.

PiperOrigin-RevId: 346097533
ulfjack pushed a commit to EngFlow/bazel that referenced this pull request Mar 5, 2021
In case that two path share a prefix, the instrumentation filter is
not computed proper.
bazelbuild#10949
This PR takes as a base the one that was created by @caiyulun and
adds some tests.
Original PR: bazelbuild#10949

Closes bazelbuild#12607.

PiperOrigin-RevId: 346097533
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-Server Issues for serverside rules included with Bazel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants