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

Add missing license for jakarta.activation against module druid-avro-extensions #13845

Merged
merged 5 commits into from Feb 26, 2023

Conversation

kfaraz
Copy link
Contributor

@kfaraz kfaraz commented Feb 24, 2023

@kfaraz kfaraz changed the title [Do Not Merge] Licenses test Add missing license Feb 24, 2023
@kfaraz kfaraz changed the title Add missing license Add missing license for jakarta.activation against module druid-avro-extensions Feb 24, 2023
@@ -56,7 +56,7 @@ jobs:
- name: packaging check
run: |
./.github/scripts/setup_generate_license.sh
${MVN} clean install -Prat -Pbundle-contrib-exts --fail-at-end \
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this change needed?

Copy link
Member

Choose a reason for hiding this comment

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

im not sure we should remove it, the packaging check was added to make sure that everything builds correctly, including contrib extensions (at least that is how i remember it)

Copy link
Contributor Author

@kfaraz kfaraz Feb 26, 2023

Choose a reason for hiding this comment

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

Keeping this profile in the command leads to misleading warning messages such as this one:
https://github.com/apache/druid/actions/runs/4261844965/jobs/7416643271#step:5:22535

Warning:  The requested profile "bundle-contrib-exts" could not be activated because it does not exist.

This is because the profile bundle-contrib-exts is present only in the distribution module and that module has been excluded in the first command here.

- name: packaging check
run: |
./.github/scripts/setup_generate_license.sh
${MVN} clean install -Prat -Pbundle-contrib-exts --fail-at-end \
-pl '!benchmarks, !distribution' ${MAVEN_SKIP} ${MAVEN_SKIP_TESTS} -Dweb.console.skip=false -T1C
${MVN} install -Prat -Pdist -Pbundle-contrib-exts --fail-at-end \
-pl 'distribution' ${MAVEN_SKIP} ${MAVEN_SKIP_TESTS} -Dweb.console.skip=false -T1C

I have retained the -Pbundle-contrib-exts for the second command, where the distribution module has been targeted.

Copy link
Member

Choose a reason for hiding this comment

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

ah, thanks I was confused, as long as it is still building them for the distribution 👍

@suneet-s
Copy link
Contributor

Forgot to ask - do you know why this check has only recently started failing?

@clintropolis
Copy link
Member

Forgot to ask - do you know why this check has only recently started failing?

I think it probably only fails in java 9+, since this is the replacement for javax.activation

@kfaraz
Copy link
Contributor Author

kfaraz commented Feb 26, 2023

I think it probably only fails in java 9+, since this is the replacement for javax.activation

Yeah, not sure why this started failing recently, but it were the jdk8 static checks that were failing.

Copy link
Contributor

@suneet-s suneet-s left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this @kfaraz !

@kfaraz
Copy link
Contributor Author

kfaraz commented Feb 27, 2023

Thanks for the reviews, @suneet-s , @clintropolis !

@clintropolis clintropolis added this to the 26.0 milestone Apr 10, 2023
@kfaraz kfaraz deleted the fix_licenses branch August 1, 2023 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants