search

lastnpe / eclipse-null-eea-augments

Eclipse External null Annotations (EEA) repository
http://lastnpe.org
Eclipse Public License 2.0
41 stars 22 forks source link

feat: integrate no-npe EEA generator #168

Closed sebthom closed 3 weeks ago

sebthom commented 3 weeks ago

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:

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 https://github.com/lastnpe/eclipse-null-eea-augments/issues/16 https://github.com/lastnpe/eclipse-null-eea-augments/issues/161

vorburger commented 3 weeks ago

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 commented 3 weeks ago
  • 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 commented 3 weeks ago
  • 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 commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

@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 commented 3 weeks ago

@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