Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add ConfigJavaInfo to jar jar aspect #51

Merged
merged 2 commits into from
Jul 18, 2024

Conversation

ramyanaga
Copy link
Contributor

Problem:
We need certain transitive runtime jars to be added to the current jars list.

Solution:
This PR creates a provider ConfigJavaInfo that a target can expose with specific jars that need to be added to current jars

@eed3si9n eed3si9n merged commit d935588 into main Jul 18, 2024
16 checks passed
@eed3si9n eed3si9n deleted the ramya/bazel_jar_jar_aspect_change_0717 branch July 18, 2024 19:27

if ConfigJavaInfo in target:
java_info_runtime_deps.append(target[ConfigJavaInfo].config_java_info)
#current_jars.extend([e.class_jar for e in target[ConfigJavaInfo].config_java_info.java_outputs])
Copy link
Collaborator

Choose a reason for hiding this comment

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

we some remove this commented code.

@@ -6,6 +6,10 @@ ShadedJars = provider(fields = [
"transitive_shaded",
])

ConfigJavaInfo = provider(fields = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

I find this name a bit confusing. I would rather name it something related to runtime_deps. But also, I'm confused why we need this... seems like we shouldn't...

Copy link
Contributor

Choose a reason for hiding this comment

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

The problem is that this rule makes the assumption that the target only generates one output JAR, and if it ever has a runtime dependency the target would have an attributed called runtime_deps:

    if hasattr(ctx.rule.attr, "runtime_deps"):
        for d in ctx.rule.attr.runtime_deps:
            if ShadedJars in d:
                shaded_jars = d[ShadedJars]
                transitive_shaded.append(depset([shaded_jars]))
                transitive_shaded.append(shaded_jars.transitive_shaded)
                java_info_runtime_deps.append(shaded_jars.java_info)

The target we have generates two output one normal JAR and the second JAR for JSON, and as far as we can tell it won't get included into the resulting JavaInfo from this aspect.

@johnynek
Copy link
Collaborator

You said: "We need certain transitive runtime jars to be added to the current jars list." Can you be more explicit?

Why do we need certain transitive runtime jars added? Is it because the aspect isn't recursing properly? Maybe that's it because the "certain jars" are coming from a rule that the aspect can't traverse? Maybe we should fix our internal rule...

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants