Skip to content

Conversation

vinnybod
Copy link
Contributor

Addresses #1106

Copy link

google-cla bot commented Apr 22, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@shs96c
Copy link
Collaborator

shs96c commented Apr 23, 2024

Thank you for the PR. It looks great, and I'll happily merge it, but could you please sign the Google CLA? If you follow the link in the Google CLA bot's comment you'll be able to do that.

@vinnybod
Copy link
Contributor Author

Thanks @shs96c , I'm just waiting on the authorized signer's signature. Hopefully will go through today.

@vinnybod
Copy link
Contributor Author

CLA is signed, but its still not going through. May still be waiting for it to be processed on Google's side

@shs96c
Copy link
Collaborator

shs96c commented Apr 24, 2024

@jin any idea why the CLA check isn't passing?

@vinnybod, the CLA check is complaining that <v***e​@confluent.io> hasn't signed the CLA. Did you use that email address? Maybe it's something to do with that?

@vinnybod
Copy link
Contributor Author

Yes, I got a corporate CLA signed and submitted and I am in the Google group.
According to https://opensource.google/documentation/reference/cla#cla-accepted , it might take a couple days for it to process.

@shs96c
Copy link
Collaborator

shs96c commented Apr 24, 2024

Great. I'll re-run the check tomorrow and see if it passes. Thank you for being patient!

@vinnybod
Copy link
Contributor Author

@shs96c I'm fine merging this since it solves my immediate issue.

But I do want to continue this discussion a bit more, because I can see myself needed to add more files to this list in the near future.

Why should file NOTICE in app-export be excluded just because it also exists in lib-export? I wonder if this is actually a bug.
It seems like if I want to include NOTICE in app-export, that should happen regardless of whether lib-export contains it or not. Does that make sense?

@vinnybod
Copy link
Contributor Author

vinnybod commented Apr 24, 2024

Actually, I think we should wait to merge this. I am seeing that in some cases this does not do what I need.
For example I have a java_export where I Add a NOTICE, but instead of my NOTICE, it is including the NOTICE from my scala dependency. I need to do some further investigation to see what is going on.

Probably safer to pause on this while i work out the issue and we look more into how the file selection happens for files with the same name.

@shs96c
Copy link
Collaborator

shs96c commented Apr 25, 2024

The LICENSE files are kept because it's often a legal requirement to keep them. The default ZipOutputStream in java forbids multiple files with the same name, but in theory what we need to do is write each license to the zip and preserve it. For Buck, I wrote a custom zip implementation that did this

Put another way, the reason you're seeing only one NOTICE file is because the underlying mechanism we're using for constructing the zip file doesn't allow us to add more than one entry with the same name.

@vinnybod
Copy link
Contributor Author

Okay, so I now believe this to be fine to merge. The issue I was running into was due to a misconfiguration with excluded_workspaces that was causing some extra files in the jars from dependents that should not have been there.

TL;DR: This solves the immediate issue of files being excluded from the -project jar when one of its deps contains a file of the same name. Treating COPYRIGHT and NOTICE files the same as we do LICENSE.

Copy link
Collaborator

@shs96c shs96c left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@shs96c shs96c merged commit b4321ee into bazel-contrib:master Apr 26, 2024
@vinnybod vinnybod deleted the vinnybod/support-copyright-notice-files-upstream branch April 26, 2024 05:58
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.

2 participants