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.

Sign up for GitHub

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

Jump to bottom

Add JaCoCo to 0.8.8 and upgrade ASM to 9.4 #17514

Closed
wants to merge 1 commit into from
Closed

Add JaCoCo to 0.8.8 and upgrade ASM to 9.4 #17514

wants to merge 1 commit into from

Conversation

somethingvague
Copy link
Contributor

Related to #16412

Commit 1 of 3

@somethingvague somethingvague mentioned this pull request Feb 16, 2023
Closed
@sgowroji sgowroji added team-Rules-Java Issues for Java rules awaiting-review PR is awaiting review from an assigned reviewer labels Feb 16, 2023
@somethingvague somethingvague force-pushed the jacoco-0.8.8-upgrade branch 2 times, most recently from 1197170 to 06689fd Compare February 16, 2023 22:38
@cushon cushon requested review from comius and hvadehra February 22, 2023 20:07
@somethingvague somethingvague changed the title Add jacoco 0.8.8 and upgrade asm to 9.3 Add JaCoCo to 0.8.8 and upgrade ASM to 9.4 Mar 13, 2023
@cushon
Copy link
Contributor

cushon commented Mar 13, 2023

Can you update this to also remove the old jacoco jars, or is there a reason that needs to happen as a follow up?

@somethingvague
Copy link
Contributor Author

Can you update this to also remove the old jacoco jars, or is there a reason that needs to happen as a follow up?

According to the README the whole change is staged, the removal of the old JARs is the last PR of 3.

Happy to change if that's incorrect?

@cushon
Copy link
Contributor

cushon commented Mar 14, 2023

Can you update this to also remove the old jacoco jars, or is there a reason that needs to happen as a follow up?

According to the README the whole change is staged, the removal of the old JARs is the last PR of 3.

Happy to change if that's incorrect?

Thanks for the context, that sounds good.

cushon
cushon approved these changes Mar 14, 2023
View reviewed changes
src/java_tools/junitrunner/java/com/google/testing/coverage/ClassProbesMapper.java Outdated
@@ -52,7 +52,7 @@ public Map<Integer, BranchExp> result() {
public ClassProbesMapper(String className) {
classLineToBranchExp = new TreeMap<Integer, BranchExp>();
stringPool = new StringPool();
className = stringPool.get(className);
this.className = stringPool.get(className);
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a separate change out for review for this which should be submitted soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I remove it from this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can leave it in for now. The fix should show up in the next hour, and if you want you could rebase at that point. But if not, the merge should be resolved when the PR is imported.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've rebased on master and the diff no longer appears for ClassProbesMapper

@cushon
Copy link
Contributor

cushon commented Mar 14, 2023

One last question--did you end up updating to JaCoCo 0.8.8, or building a later version? Either one is OK with me, but if it's a newer version please mention that in the PR somewhere.

@somethingvague
Copy link
Contributor Author

One last question--did you end up updating to JaCoCo 0.8.8, or building a later version? Either one is OK with me, but if it's a newer version please mention that in the PR somewhere.

No, I tried to keep as close to a release version as possible. These JARs are tag 0.8.8 (c3d5e0ff638caf0038ce46054b906d51d59623e6) with third_party/java/jacoco/0001-Partially-revert-40c8fd89b0bc0c36e30e6a12f5b42d9da13.patch applied on top.

@cushon cushon added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 14, 2023
@somethingvague
Upgrade JaCoCo to 0.8.8 and ASM 9.4
3083a16 
+ ClassProbesMapper fix to initialize the className correctly

Related to bazelbuild#16412
copybara-service bot pushed a commit that referenced this pull request Mar 15, 2023
@somethingvague
Upgrade JaCoCo to 0.8.8 and ASM 9.4
5030e89 
+ ClassProbesMapper fix to initialize the className correctly

Related to #16412

Partial commit for third_party/*, see #17514.

Signed-off-by: kshyanashree <kshyanashreem@example.com>
@ShreeM01
Copy link
Contributor

Merged at 5030e89

@ShreeM01 ShreeM01 closed this Mar 15, 2023
@ShreeM01 ShreeM01 removed awaiting-review PR is awaiting review from an assigned reviewer awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally labels Mar 15, 2023
fweikert pushed a commit to fweikert/bazel that referenced this pull request May 25, 2023
@somethingvague @fweikert
Upgrade JaCoCo to 0.8.8 and ASM 9.4
c95b6f1 
+ ClassProbesMapper fix to initialize the className correctly

Related to bazelbuild#16412

Partial commit for third_party/*, see bazelbuild#17514.

Signed-off-by: kshyanashree <kshyanashreem@example.com>
@hvadehra hvadehra mentioned this pull request Jan 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cushon cushon cushon approved these changes

@comius comius Awaiting requested review from comius

@hvadehra hvadehra Awaiting requested review from hvadehra

Assignees
No one assigned
Labels
team-Rules-Java Issues for Java rules
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@somethingvague @cushon @ShreeM01 @sgowroji