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 config output jars to current_jars #53

Merged
merged 3 commits into from
Jul 19, 2024

Conversation

ramyanaga
Copy link
Contributor

@ramyanaga ramyanaga commented Jul 19, 2024

This change does the following:

  • rename ConfigJavaInfo to ExtraDependencyProviders and rename the field config_java_info to extra_java_deps
  • update the type of extra_java_deps from single struct to list of structs + update the code to handle the list
  • add the class jars from extra_java_deps to current_jars

@johnynek
Copy link
Collaborator

Can we find a better name than ConfigJavaInfo.

Something like ExtraDependencies or something? I don't know.

@ramyanaga ramyanaga merged commit ea3ecc1 into main Jul 19, 2024
16 checks passed
@ramyanaga ramyanaga deleted the ramya/add-config-jars-to-current-jars branch July 19, 2024 17:09
Comment on lines +72 to +73
java_info_runtime_deps.append(config.extra_java_deps)
current_jars.extend([e.class_jar for e in config.extra_java_deps.java_outputs])
Copy link
Contributor

@eed3si9n eed3si9n Jul 19, 2024

Choose a reason for hiding this comment

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

These are somewhat conflicting adds.

  1. java_info_runtime_deps would bypass the jarjar (Jar Jar Abrams), and will NOT be shaded thus I've suggested the name ConfigJavaInfo indicating this should contain text files only.
  2. current_jars would go through the jarjar, and the contents will be subject to jarjar rule. I guess we've confirmed that JSON would pass through, so should be ok.

I think having both are wrong here.

# 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