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.

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

Propagate aspect to generating rules by default #6

Merged
merged 1 commit into from
Oct 29, 2021

Conversation

ulfjack
Copy link
Contributor

@ulfjack ulfjack commented Oct 25, 2021

This should make the aspect more reliably collect licenses from included files.

  • Tests pass
  • Tests and examples for any new features.
  • Appropriate changes to README are included in PR

This should make the aspect more reliably collect licenses from included files.
@ulfjack ulfjack requested a review from aiuto as a code owner October 25, 2021 23:06
@ulfjack
Copy link
Contributor Author

ulfjack commented Oct 25, 2021

Maybe setup GitHub Actions?

@aiuto
Copy link
Collaborator

aiuto commented Oct 29, 2021

I'll take this PR for now, but without great test cases (we have some, just not ready for public) it's not clear if it will stick.

@aiuto aiuto merged commit 683e47b into bazelbuild:main Oct 29, 2021
@ulfjack ulfjack deleted the patch-1 branch October 29, 2021 12:49
@ulfjack
Copy link
Contributor Author

ulfjack commented Oct 29, 2021

Hi @aiuto!

We aren't using this code at all. I wrote an aspect for our own use that works with our current license definitions and I've been diffing the result against our previous license collection implementation to check if we're missing anything. That said, I was hoping that we could eventually migrate to a common implementation but it doesn't currently look like it.

This implementation has several shortcomings that make it unusable for us:

  • it didn't collect through output file dependencies (fixed in this PR)
  • we have a custom license type that also has attached file references to license and notice files as well as URLs pointing at the original source (this information is required for some cloud marketplace deployments)
  • the list of attributes it traverses is too narrow; it's actually hard to come up with a complete list, but here's what we're using for a Java application: "deps", "exports", "jars", "resources", "runtime_deps", "srcs"; for other languages, we may need to collect from even more attributes (e.g., hdrs for C++)
  • it doesn't enforce our repo-specific annotation rules: we're enforcing license annotations for all rules under our third_party directory and a subset of external workspaces; some of that is specific to our use case

I primarily sent you a patch for this to help prevent other people going down the rabbit hole of figuring out how to collect licenses through output file dependencies.

Thanks!

-- Ulf

@ulfjack ulfjack mentioned this pull request May 11, 2022
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.

None yet

2 participants