Fix the LICENSE/NOTICES files with the missing dependencies/versions#16043
Fix the LICENSE/NOTICES files with the missing dependencies/versions#16043jbonofre wants to merge 1 commit intoapache:1.10.xfrom
Conversation
|
I suppose a lot of dependencies are missing from |
|
@manuzhang I updated the LICENSE/NOTICE in the bundles and flink/spark runtimes. I'm gonna check |
|
@manuzhang I updated |
|
Codex found 61 resolved module names missing from open-api/LICENSE. Confirmed packaged examples include (I verified some as well):
High: Several entries that are present still have stale versions compared with the same resolved runtime classpath. Examples:
|
|
@manuzhang did you check on the 1.10.x branch? I'm surprised: for instance, jackson is 2.18.x on the branch. This PR is on the 1.10.x branch. I forgot to push the new entries, let me do that. |
|
@manuzhang then, I suspect some other |
|
@manuzhang open-api |
|
@jbonofre We are getting closer but still some entries are missing and some stale. Missing license entries:
Stale license entries:
|
|
@manuzhang good catch. I'm updating. |
|
@manuzhang here we are 😄 It should be good now. |
|
@jbonofre One final comment open-api/LICENSE:2000 includes org.junit.platform:junit-platform-launcher and the junit-platform-suite-* entries, but those are not in testFixturesRuntimeClasspath, |
|
@jbonofre, this repo doesn't use the |
|
@rdblue absolutely. Thanks for the reminder! |
|
@jbonofre @rdblue I also thought before we did all the LICENSE/NOTICE changes we were first going to do a vetting of all the dependencies we're shipping and scope that down wherever applicable? Then we'd update LICENSE/NOTICE. |
|
@amogh-jahagirdar my understanding is to do that on main. The dependencies set should not have changed on 1.10.x. But if you think so, I'm happy to do an analysis. |
|
@rdblue PR title updated. |
|
Thanks @jbonofre so there were dependency upgrades to address CVEs (like the jackson upgrade that @manuzhang referenced earlier). Theoretically those patch releases shouldn't change anything but my understanding was any dependency upgrade needs to be scrutinized, or maybe my understanding there was wrong? |
|
|
||
| -------------------------------------------------------------------------------- | ||
|
|
||
| Group: com.github.ben-manes.caffeine Name: caffeine Version: 2.9.3 |
There was a problem hiding this comment.
Versions get stale and should not be included in the license files.
There was a problem hiding this comment.
That's the change I did on main, but for 1.10.x, I keep the format consistently.
There was a problem hiding this comment.
Sounds reasonable to me, thanks for the context.
|
|
||
| Apache Iceberg | ||
| Copyright 2017-2025 The Apache Software Foundation | ||
| Copyright 2017-2026 The Apache Software Foundation |
There was a problem hiding this comment.
This should be done in a separate PR. PRs should generally just do one thing.
There was a problem hiding this comment.
As I consider it's a fix on the NOTICE, I included there. But fine to split.
There was a problem hiding this comment.
I gonna split in two PRs: this one will content the minimal change to have the LICENSE/NOTICE correct (the PR title will be updated accordingly). Let me create another PR specific to copyright year update. Thanks for the suggestion @rdblue !
| Copyright: 2014-2024 The Apache Software Foundation | ||
| Home page: https://parquet.apache.org/ | ||
| License: http://www.apache.org/licenses/LICENSE-2.0 | ||
| License: Apache License, Version 2.0 - http://www.apache.org/licenses/LICENSE-2.0.txt |
There was a problem hiding this comment.
These changes are unnecessary and shouldn't be included here. We can talk about whether to have more information on this line, but it isn't related to the purpose of this PR, which is to update the files for correctness. Including unnecessary changes increases what needs to be reviewed (or in this case scrolled past) and that can cause things to be missed.
There was a problem hiding this comment.
It was a request from @manuzhang for consistency.
I'm fine to revert.
There was a problem hiding this comment.
These are large enough without other changes. Style updates like this should be in separate PRs. Thanks!
There was a problem hiding this comment.
Sorry, I feel the pain to go back and forth in such a large PR. Maybe we can just leave out the style issues for the patch release?
There was a problem hiding this comment.
That makes sense to me (and it was my initial change 😄 ). I propose to take a step back here with the following:
- Let me list the dependencies in each "modules" (bundle and runtime artifacts) with a first pass for dependencies requiring "discussions" (I'm thinking of Eclipse Collections for instance).
- I'm updating this PR with the minimal changes to have LICENSE/NOTICE files correct.
- Depending of our finding on 1, I will update the PR accordingly.
Does it work for you guys?
Thanks again for your help on this one!
|
|
||
| -------------------------------------------------------------------------------- | ||
|
|
||
| This binary artifact contains Eclipse Collections. |
There was a problem hiding this comment.
How is this used? Is it pulled in transitively?
We don't use Eclipse collections, so if this is a new direct dependency, we should remove it.
Also, for Spark we want to check what Jars are already included in the Spark bundle and exclude them so that we don't ship duplicates or conflicts.
There was a problem hiding this comment.
It's directly bundled in the runtime jar:
....
2913 Fri Feb 01 00:00:00 CET 1980 org/eclipse/collections/api/LazyIntIterable.class
2961 Fri Feb 01 00:00:00 CET 1980 org/eclipse/collections/api/LazyFloatIterable.class
2937 Fri Feb 01 00:00:00 CET 1980 org/eclipse/collections/api/LazyLongIterable.class
2985 Fri Feb 01 00:00:00 CET 1980 org/eclipse/collections/api/LazyDoubleIterable.class
13693 Fri Feb 01 00:00:00 CET 1980 org/eclipse/collections/api/ParallelIterable.class
This is coming from arrow-vector (transitive).
I believe that it has not been introduced for 1.10.2 but before.
There was a problem hiding this comment.
Can we just fix license issues in the patch release and fix dependency issues on the main branch?
|
@amogh-jahagirdar I think it's reasonable indeed. I also think that several issues exist for a while and probably need a cleanup (I'm thinking on Eclipse Collections for instance). |
Sorry, I should have been more clear. This project doesn't follow the "conventional commits" conventions for PR titles. I'd be fine having a discussion about this in the community, but I don't think that it is helpful to have conflicting conventions. The convention that we follow is to list the high-level areas that are being changed, like "API" and "Core", or "Spark 4.1", followed by a colon, and then a short and direct description. |
|
@rdblue ok, good. I'm surprised it comes now because I saw several PRs (including mine) using conventional commits style (that's why I was confused, sorry about that). Fair enough, I'm updating to use the recommended format. Thanks! |
|
@manuzhang @rdblue @amogh-jahagirdar I changed the PR to focus on the minimal changes needed to fix I'm preparing the dependencies list for each artifact (bundle and runtime) identifying the dependencies worth to discuss (probably excluding not necessary ones and so updating |
That might be that other iceberg projects are following conventional commits style. I created #16101 to add PR title check workflow. |
I'd suggest fixing dependencies issue just on the main branch, since it could be too many dependency changes for a bugfix release. |
This closes #16013