search

apache / netbeans

Apache NetBeans
https://netbeans.apache.org/
Apache License 2.0
2.64k stars 849 forks source link

OpenJDK download plugin #2854

Closed emilianbold closed 3 years ago

emilianbold commented 3 years ago

foojay would like to include into NetBeans a plugin that allows users to download JDKs.

Users can pick one of the many OpenJDK distributions, download it and then auto-register it with NetBeans (or allow the users to manually install it globally).

The dependencies are Apache, BSD and MIT.

The code itself is Apache licensed and this commit just includes the code from https://github.com/foojay2020/nbplugin2/ If more paperwork is required by the PMC then I think @geertjanw with the Azul hat can clarify privately.

The plugin needs this JAR file uploaded to OSUOSL https://github.com/foojay2020/nbplugin2/blob/main/foojaysupport/release/modules/ext/discoclient-1.0.jar under the name 6E147B7F363AD52D6BD10C32C64A273FA07F7EED-discoclient-1.0.jar The code for this plugin is at https://github.com/foojay2020/discoclient/tree/java8 but since it's not published in Maven Central we have to include it through OSUOSL.

I think this is a great feature for NetBeans end users!

The plugin is not perfect and feedback is appreciated. It will be maintained by me / foojay going forward.

Screenshot 2021-03-31 at 19 02 11 Screenshot 2021-03-31 at 19 03 48
geertjanw commented 3 years ago

Great. As someone also involved with this plugin, I'm very happy this has made it to a pull request.

I'd be in favor of including it in 12.4, though too late for the Beta scheduled for today, though this is before feature freeze so it would be valid for this feature to make it in time for the first release candidate.

emilianbold commented 3 years ago

PS: All builds fail since OSUOSL doesn't have 6E147B7F363AD52D6BD10C32C64A273FA07F7EED-discoclient-1.0.jar yet.

lbruun commented 3 years ago

This is awesome !!

I understand that the plugin uses "Universal OpenJDK Discovery API". What is that? Is it an Azul API or an foojay endavour? (a link would be good)

Is that something which OpenJDK distros have adopted? (i.e. the Amazon Corretto, the AdoptOpenJDK, the Oracle/GraalVM, and what not). In other words : which distros can currently be discovered and auto-downloaded by the plugin, besides Zulu ?

geertjanw commented 3 years ago

It is an API developed at Azul (by my team there) that unifies all existing APIs (and more) for discovering OpenJDK distros.

It is used by SDKMAN, it's been introduced to different IDE vendors, there's a PR for integration into setup/java GitHub Action, and now here it is for NetBeans, thanks to Emilian who did the integration work.

Just like other features in NetBeans (e.g., the Payara integration), it depends on an external API, which is the Disco API, the vendor neutral OpenJDK Discovery Service.

emilianbold commented 3 years ago

@lbruun it has many (all?) OpenJDK distros. See

Screenshot 2021-02-26 at 10 24 47
geertjanw commented 3 years ago

And any that are missing, welcome to create an issue for adding those at github.com/foojay2020.

The purpose is to have all of them, though some balance needs to be found, e.g., I could create my own personal OpenJDK distro right now but maybe that random OpenJDK distro shouldn't be offered to users, at least not without a warning. Maybe TCK compliance should be the standard, but then AdoptOpenJDK would be excluded.

There are no easy answers. :-)

lbruun commented 3 years ago

@emilianbold Excellent. More than happy with what is there. My beef - in the past - has been with vendors which did not include all sources, but only the "public" interfaces. Arghh!! Azul gets it right. Oracle's commercial JDK doesn't. I assume the plugin will download and register Sources and Javadoc too .. if the vendor publishes them?

emilianbold commented 3 years ago

I think the sources are auto-registered but the Javadoc is not. I will create an issue about Javadoc since I forgot. I suspect though that not all vendors to publish javadoc...

geertjanw commented 3 years ago

The JAR will be put on Maven Central, though having it on OSUOSL will be a nice temporary solution so that people can start trying this out in the meantime -- not as part of 12.4 Beta but in the release candidate.

Thanks for getting this in before feature freeze, @emilianbold.

neilcsmith-net commented 3 years ago

Would be a great feature to get in (and great to see the discussion on Disco at FOSDEM). But, sorry, -1 in current state.

This might be better as a plugin in the 12.4 timescale?

geertjanw commented 3 years ago

Here's @emilianbold and my request for the io.foojay Maven namespace:

https://issues.sonatype.org/browse/OSSRH-66676

emilianbold commented 3 years ago 
* We can't just merge code developed in a different repository into NetBeans.  Is it being donated?

Presumably every PR starts as code developed in a different repository.

Not sure where the line between a plain contribution and a donation is so please talk to Geetjan within the PMC. I do have his approval to create this PR for foojay but I don't own the entirety of this code so I don't know what the formalities are.

* Missing license headers.

Hardly an impossible task.

* Should it be refactored into `org.netbeans` namespace?

Not sure. But minor thing for a plugin with no public APIs.

* Would prefer we weren't using OSUOSL for the dependency.

As a matter of taste so would I but also a minor thing to upload a JAR to OSUOSL until it gets into Maven Central. Note the JAR is also from foojay so there's no other 3rd party involved.

geertjanw commented 3 years ago

The code is mostly written by Emilian and a bit by me. If anything, we're both working for my company Gj IT. If I need to donate the code on behalf of my company, no problem.

matthiasblaesing commented 3 years ago

I did a quick(!) pass over this and noticed, that indeed licenses are missing everywhere. The package namespace also looks strange for an in tree plugin.

If this is indeed not implemented by @emilianbold and @geertjanw alone, but by a company, a CCLA would be needed (if the two of you consider other contributions minimal, you can claim authorship and it would be covered by the ICLA)

geertjanw commented 3 years ago

I think we should add license headers and fix the package namespace.

Both Emilian and I can contribute the code under our ICLA and for any code by Gerrit, we can ask him to officially donate it.

We have a request for io.foojay namespace on Maven and I suggest in the interim we put the client JAR on OSUOSL.

We'll leave these tasks to be done, i.e., we're not trying to include this in today's Beta, but will aim to have everything ready for merging by feature freeze.

It would be appreciated if anyone would look at the code itself to see if there are any problems.

How does the above sound? Any issues with the above process, please speak up. :-)

neilcsmith-net commented 3 years ago

Sounds good! I would also note that we don't need an ICLA/CCLA for every contribution, but that the committer who merges also takes responsibility for ensuring the provenance of what is merged.. So, once other issues addressed, would make sense for @geertjanw to handle that?

geertjanw commented 3 years ago

io.foojay now exists on Maven Central and so @emilianbold can publish the JAR there:

https://issues.sonatype.org/browse/OSSRH-66676

emilianbold commented 3 years ago

The PR has been updated to:

geertjanw commented 3 years ago

@emilianbold, can you check why some of the checks were not successful?

emilianbold commented 3 years ago

I dunno, I briefly looked and errors seemed to happen in the 'clean' task or some other places which can't possibly be related to the PR so I concluded something is off with the build infra.

I may be wrong, but this was my quick estimation.

Will revisit tomorrow if it doesn't recover or if this issue doesn't get some other replies.

--emi

dum., 4 apr. 2021, 10:36 Geertjan Wielenga @.***> a scris:

@emilianbold https://github.com/emilianbold, can you check why some of the checks were not successful?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/apache/netbeans/pull/2854#issuecomment-812989065, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHSCQWYG2KDRVEOI53P4LDTHAJGXANCNFSM42IMLVMA .

neilcsmith-net commented 3 years ago

Verify libs and licenses is still failing https://travis-ci.com/github/apache/netbeans/jobs/495872110#L1293

emilianbold commented 3 years ago

Verify libs and licenses is still failing https://travis-ci.com/github/apache/netbeans/jobs/495872110#L1293

Thanks. So I need to:

matthiasblaesing commented 3 years ago

For the overlapping binaries problem please see nbbuild/antsrc/org/netbeans/nbbuild/extlibs/ignored-overlaps to define an exception. If multiple modules need the same binary a common wrapper module should be created to hold that dependencies and the others depend on that module. And yes, the headers of the license file need to be separated from the actual license text by an empty line.

neilcsmith-net commented 3 years ago

Thanks for changes. I retriggered a couple of tests on Travis which can be a little unstable. However, I think the commit-validation might be legit - could you take a look?

Assuming we get tests green, and @geertjanw handles merging, consider my -1 withdrawn.

emilianbold commented 3 years ago

Indeed, I'm not entirely sure how the plugin update should be done. Should the plugin be part of java.kit or something else or should it have AutoUpdate-Essential-Module=true (which I never used honestly)?

JaroslavTulach commented 3 years ago

Dependency from java.kit is one (not very nice, as it makes the module kind of required) option. The other one is to make this module is.autoload=true and provides a token that java.platform module requires:

OpenIDE-Module-Recommends: org.netbeans.modules.java.platform.ui

Try this provides/recommends approach, please.

emilianbold commented 3 years ago

Thank you @JaroslavTulach, I've done that. Hopefully the builds will be pleased with the PR.

JaroslavTulach commented 3 years ago

Hopefully the builds will be pleased with the PR.

You can always verify locally using ant commit-validation.

emilianbold commented 3 years ago

I think the last failing test is not caused by my PR. I see

9029.20 Test Java modules without nb-javac on Java 14

is failing

emilianbold commented 3 years ago

All checks pass!

Does this mean the PR is finally mergeable?

geertjanw commented 3 years ago

I'd say, yes. Unless someone objects, I will merge 24 hours from now.

neilcsmith-net commented 3 years ago

I retriggered the failing test the other day. I'm happy if @geertjanw merges (from provenance point of view, not tested functionality). @matthiasblaesing ?

geertjanw commented 3 years ago

OK, merging.