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

feat: integrate no-npe EEA generator #168

Merged
merged 2 commits into from
Aug 30, 2024
Merged

feat: integrate no-npe EEA generator #168

merged 2 commits into from
Aug 30, 2024

Conversation

sebthom
Copy link
Member

@sebthom sebthom commented Aug 26, 2024

With this PR Vegard IT GmbH https://vegardit.com/ contributes the code of it's EEA Generator/Validator hosted at https://github.com/vegardit/no-npe to the LastNPE project.

The following changes are made to the project:

  • the com.vegardit.no_npe.eea_generator EEAGenerator is migrated to the lastnpe project under the root package org.lastnpe.eea.generator
  • the maven project structure and configuration is updated accordingly
  • outdated dependencies and plugins are updated
  • the usage documentation is extended 77a4946
  • CI job is configured that validate EEAs and generate a maven snapshot repo at https://github.com/lastnpe/eclipse-null-eea-augments/tree/mvn-snapshots-repo

You can see how the generated EEA jar files will look like at https://github.com/sebthom/eclipse-null-eea-augments/tree/mvn-snapshots-repo/org/lastnpe/eea

This PR solves #16 #161

@vorburger
Copy link
Member

With this PR Vegard IT GmbH contributes the code of it's EEA Generator/Validator hosted at https://github.com/vegardit/no-npe to the LastNPE project.

Thank You! I'm personally excited to see this ... rejuvenation of this project!

I would love to hear from any of the people who were active here in the past, if they are all for this as well, or anyone got any sort of objections? I doubt it - so just asking for good community stewardship! Paging, looking at https://github.com/lastnpe/eclipse-null-eea-augments/graphs/contributors and https://github.com/orgs/lastnpe/people, e.g. @J-N-K and @rPraml and @focbenz and @maggu2810 and @ctron
ctron and @triller-telekom and @sylvainlaurent and @Bananeweizen and @wborn and @agentgt and @arathai and @brychcy and @kaikreuzer, as a PSA Public Service Announcement... silence in the next 1 week is assumed to imply no objections! 😸 (Also /CC @cpovirk @msridhar just as an FYI.)

We are aware that this is a large PR but ask you to merge it as is if possible. We can later finetune things, extend documentation, solve nitpicks etc.

At least my browser is crushing on the Files changed tab... 😄 I don't suppose that I could motivate you to break this up, at least a little bit? E.g. along the lines of something like, just a thought:

  1. infra / build stuff, to fix CI again and test (initially small / empty) new maven snapshot repo
  2. docs related (e.g. there's something I'd like to comment on in the proposed changes to CODE_OF_CONDUCT.md - but I literally cannot!)
  3. tooling Java code
  4. actual new EEA - perhaps in a few "batches"?

This PR solves #16

I would love to understand, but still don't really, how - but let's further discuss this there, not here! (Not holding this up on that.)

@J-N-K
Copy link
Member

J-N-K commented Aug 26, 2024

  • the javax servlet/mail modules are migrated to jarkarta

Does that mean EEAs for javax.servlet and javax.mail are removed? Especially javax.servlet is used in our project. I'm fine with merging here and re-adding it later. I would object if re-adding is not possible.

@sebthom
Copy link
Member Author

sebthom commented Aug 26, 2024

  • the javax servlet/mail modules are migrated to jarkarta

Does that mean EEAs for javax.servlet and javax.mail are removed? Especially javax.servlet is used in our project. I'm fine with merging here and re-adding it later. I would object if re-adding is not possible.

@J-N-K I have no strong feelings about this. In https://github.com/vegardit/no-npe/ I maintain EEAs for the javax and the jakarta version. But I think we should generally come up with some guideline which frameworks/libraries we maintain and when to drop them. After all one can always reference the already published artifacts like https://central.sonatype.com/artifact/org.lastnpe.eea/servlet-api-eea/2.4.0 even if they libs are removed from latest releases. Since javax.servlet was superseded by jakarta.servlet 4 years ago it might be a good point in time to drop it with a new upcoming major release of lastNPE.

@J-N-K
Copy link
Member

J-N-K commented Aug 27, 2024

Karaf - being one of the most popular OSGi frameworks - still depends on PaxWeb 8 / Jetty 9.x and that requires javax.servlet, that's why we need it. IMO we should still support it. But as I said: If there is no technical issue to add it, we can merge here, discuss and add it back later.

@vorburger
Copy link
Member

vorburger commented Aug 27, 2024

Re. javax.servlet vs. jakarta.servlet, why don't we just keep the old existing one as-is? No harm, no? I'm not sure that I see a strong reason to delete it here? So more like what you seem to have done for eea-java-11 / eea-java-17 / eea-java-21... why not instead of renaming that similarly keep e.g. eea-javax-servlet AND eea-jakarta-servlet? With version prefixes, if needed, sure. As for "double maintenance", I think we can just do "who needs & uses, maintains it!" 😃 here, agreed?

More generally, @sebthom do you want to further break up what's on this (too?) massive PR, so that it's easier to iteratively review more batches? I'm still reluctant to LGTM this for 5000+ files changed which I'm struggling to (easily) review.

@sebthom
Copy link
Member Author

sebthom commented Aug 27, 2024

@vo

Re. javax.servlet vs. jakarta.servlet, why don't we just keep the old existing one as-is? No harm, no? I'm not sure that I see a strong reason to delete it here? So more like what you seem to have done for eea-java-11 / eea-java-17 / eea-java-21... why not instead of renaming that similarly keep e.g. eea-javax-servlet AND eea-jakarta-servlet? With version prefixes, if needed, sure. As for "double maintenance", I think we can just do "who needs & uses, maintains it!" 😃 here, agreed?

it's fine with me.

More generally, @sebthom do you want to further break up what's on this (too?) massive PR, so that it's easier to iteratively review more batches? I'm still reluctant to LGTM this for 5000+ files changed which I'm struggling to (easily) review.

Yes, I see how we can break it up further. We are already down 10 commits :-)

@sebthom
Copy link
Member Author

sebthom commented Aug 27, 2024

@vorburger I stripped down the PR and will submit the other changes in separate PRs.

With these changes you should be able to run mvn clean package. This will run the EEA Generator in validation mode across all libraries (except the javamail and servletapi libs which are commented out for the moment). It generate jars with the EEAs plus a manifest file with the Eclipse-ExportExternalAnnotations: true header.

The included CI job validates EEAs for each commit/PR and will create/update a Maven snapshot repository when run on the main branch

@sebthom sebthom force-pushed the main branch 5 times, most recently from 801e4ac to 3ef257b Compare August 27, 2024 19:20
@vorburger vorburger mentioned this pull request Aug 29, 2024
pom.xml Show resolved Hide resolved
Signed-off-by: sebthom <sebthom@users.noreply.github.com>
Signed-off-by: sebthom <sebthom@users.noreply.github.com>
@vorburger vorburger merged commit 98f3d86 into lastnpe:main Aug 30, 2024
1 check passed
vorburger added a commit that referenced this pull request Aug 30, 2024
Motivated by related discussions during review of #168.

Signed-off-by: Michael Vorburger <mike@vorburger.ch>
J-N-K pushed a commit that referenced this pull request Aug 31, 2024
Motivated by related discussions during review of #168.

Signed-off-by: Michael Vorburger <mike@vorburger.ch>
@J-N-K J-N-K added this to the 3.0.0 milestone Aug 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants