Skip to content

Fix common prefix for instrumentation filter#12607

Closed
limdor wants to merge 1 commit intobazelbuild:masterfrom
limdor:fix_path_instrumentation_filter
Closed

Fix common prefix for instrumentation filter#12607
limdor wants to merge 1 commit intobazelbuild:masterfrom
limdor:fix_path_instrumentation_filter

Conversation

@limdor
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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