Skip to content

Conversation

@brian-mcnamara
Copy link
Contributor

See https://github.com/bazel-contrib/rules_jvm/pull/303/files/d899e276ff1a4a9b98272ed6cd31991f09d7839a#r2042301337 for discussion. This change removes library_attrs from the two "classpath healper" libraries (which were introduced as a classpath simplification for bazel), as well as poping the resources from the kwargs if the resources library will include them.

Also added a test case for this

@honnix
Copy link
Contributor

honnix commented Apr 21, 2025

I think I'm seeing some weird issue when applying this patch. It feels like resources are not picked up.

@honnix
Copy link
Contributor

honnix commented Apr 21, 2025

I think I'm seeing some weird issue when applying this patch. It feels like resources are not picked up.

I know what happened. There are cases we intentionally use a resource named exactly the same as that in production, in order to override. Since -test-lib.jar appears pretty late in classpath (after the production jar), the overridden was not picked up. I think maybe -test-lib.jar should not pack any test resources.

@brian-mcnamara
Copy link
Contributor Author

I think I'm seeing some weird issue when applying this patch. It feels like resources are not picked up.

I know what happened. There are cases we intentionally use a resource named exactly the same as that in production, in order to override. Since -test-lib.jar appears pretty late in classpath (after the production jar), the overridden was not picked up. I think maybe -test-lib.jar should not pack any test resources.

Another solution would be to set library_attributes to not include resources. My thinking is that this would reduce the duplication of resources being added to each jar. Thoughts?

@honnix
Copy link
Contributor

honnix commented Apr 21, 2025

Another solution would be to set library_attributes to not include resources. My thinking is that this would reduce the duplication of resources being added to each jar. Thoughts?

Yeah that was what I suggested, and yes the trade-off is duplicating resources in each every test jar.

@honnix
Copy link
Contributor

honnix commented Sep 3, 2025

Totally forgot about this. I think this is still valid, right?

@brian-mcnamara
Copy link
Contributor Author

Ooph, I too forgot about this, it should still be valid, looks like I need to resolve a conflict tho, let me get to that

@shs96c
Copy link
Collaborator

shs96c commented Sep 10, 2025

@brian-mcnamara, could you please rebase on main and do a push. That should fix the failing test so we can get this merged. My apologies it's taken so long to get to this.

@brian-mcnamara
Copy link
Contributor Author

Merged in the latest, will check back on the CI. No worries on the delay, frankly I too forgot about this 🙈

@brian-mcnamara
Copy link
Contributor Author

Looks like everything is healthy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants