-
Notifications
You must be signed in to change notification settings - Fork 784
support external annotations #4217
support external annotations #4217
Conversation
What do you think guys? |
Perfect! Thank you very much for integrating them! I just had a look on this PR and in the IDE all seems fine. For testing I created a function
in
from But I maven this warning doesn't even appear, with or without the external annotations. Did you make some tests with maven and what were your results? One question regarding the IDE support for adding additional annotations: The documentation you are linking to says that we need to have the source code for that library attached. So hoe did you configure your IDE to attach the source of the JDK? Can we have that as a general setting for everyone? |
IMHO it generally makes sense in order to complete the null-annotations picture! I'm just wondering whether it makes sense to start from scratch or if it rather makes sense to join forces with e.g. https://github.com/lastnpe/eclipse-null-eea-augments. It's not big yet, but it's a start and it somehow feels inefficient if every project again and again creates eea files for popular libs. Are you aware of it? |
I installed the sources with every JDK on my machine and its always configured in the Eclipse IDE. So I haven't changed anything. I didn't tound any external annotation project. Is this a well known one? Where is it linked? |
@maggu2810 Could you please give some insights on how you tested the external annotations with maven? I.e. do you have an example where maven now reports a warning which then disappears if you enable the external annotations? |
@maggu2810 See here the start of that discussion: #3933 (comment)
Well, it has been created by a good friend of mine because nothing adequate existed. Check his presentation from EclipseCon Europe last year (btw, this has interesting infos that might help us resolving our general open discussion points around null annotation usage). He worked on this jointly with Stephan Hermanns, the guy who has implemented the whole null annotation support in the Eclipse IDE - so I guess this is the closest to an "official" repo as we can get... |
@triller-telekom Sorry, for the first step I didn't test it really. |
Strange... I just tested the following and it works as expected: Added method in ThingType.java
With Collections.eea as included in this PR everything is fine in IDE and maven. If I remove
from this file then I get a warning in the IDE on this maven result:
So all is working as expected? |
pom.xml
Outdated
@@ -71,6 +71,23 @@ | |||
<build> | |||
<plugins> | |||
<plugin> | |||
<groupId>org.commonjava.maven.plugins</groupId> | |||
<artifactId>directory-maven-plugin</artifactId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to wait for the CQ, see also #4240
Hopefully, if something is not working as expected, we could fix it later, too. But we need to wait for the CQ. |
I do not think that this is the best way to move forward.
For the first group, requiring the eeas on the local file system / within the workspace is imho a no-go. Note that most binding developers e.g. work with the openHAB 2 IDE, which (normally) does not require a checkout of the ESH project at all for development. In general, it goes very much against any "minimal IDE requirements" (see also regular questions about compiling against pure Maven ESH artefacts / bnd support / etc.) to require such a project in the workspace. For the second group: This is really nothing specific about ESH, so I do not see why this should be specifically maintained in our repo. As @SJKA pointed out above, there is already a place that intended to be the shared place for the whole community. Did you already check whether it could be an option for us to participate in it? |
We are talking about external annotations here for third party libraries that are used by the ESH bundle developement. This does not need to be the same third party bundles that products consuming ESH bundles. For consuming ESH bundles EEA shouldn't be necessary. Or do I miss something here. Or do we need to maintain the external(!) non-ESH annotations for non-ESH bundles, too? Have you ever looked at the number of EEA in the repo @SJKA? At which place do you think to host that eea-jars so it could on build time? Feel free to create a PR against my branch if you found a suitable solution. |
We could create an artifact from the eclipse-external-annotations so you could use it in openHAB (for the consumers you pointed in (1)). For ESH it would be (at least the first months) better to host it in the ESH repo as after some hours of usage you will add a lot of annotations. Moving this to an external one should be done after no more or rare annotations of JRE are added. |
Hello! I'd love to get you guys involved in lastnpe.org... Didn't have much time for it recently, but have been meaning to pick it up, so this is perfect timing. I'll be merging pending PRs ASAP, and publish that Eclipse plug-in for M2E integration somewhere. Perhaps you guys would like to contribute your EEAs there? |
@vorburger The contribution of the EEA is no problem. Regardless of the decision we take here, I will create a PR for the Java 8 EEA to your repository. But for the current situation I don't think it would be a good decision to use the EEAs of an external repository. |
@maggu2810 I think we agree that the released artifact is only relevant for the "consumers" - releasing such a jar to Maven central would actually much easier from lastNPE than doing that from ESH.
That's an interesting question. @vorburger are the annotations specific for a certain version of a lib? In https://github.com/lastnpe/eclipse-null-eea-augments/tree/master/libraries/guava, I cannot find any information about which Guava version the annotations are actually meant for. So how could lastNPE possibly be a globally shared repo if everyone uses different versions of the libs? |
@vorburger What is the correct place to ask you some questions related to the project?
|
Confirming - that's still very much my intention. @kaikreuzer you've just been invited as committer... ;-)
lastnpe/eclipse-null-eea-augments#14 ?
https://github.com/lastnpe/eclipse-external-annotations-m2e-plugin ?
The intention is Yes, but we probably haven't completely thought this through yet. Open an issue, and help? :)
https://github.com/lastnpe/eclipse-null-eea-augments/issues
Your Contributions to some of the ideas in the TODO and/or open GitHub issue are most welcome! ;-)
Someone needs to write a tool that converts Checker annotations to Eclipse.org JDT EEA format. Maybe you @maggu2810 ?
==> lastnpe/eclipse-null-eea-augments#10
hopefully the slides from my EclipseCon presentation can help you. I'm afraid I'll not have the bandwith to actually get involved in ESH to set it up for you. I'm however hoping to find the time to integrate lastenpe.org with another project I'm involved in by day for work, so may be once that is done it will be useful to you to look at that; see e.g. https://git.opendaylight.org/gerrit/#/c/47370/. |
@vorburger I have edit my comment last night again, so I assume you didn't get a mail. Can you please have a look at the edit post again? |
It would be great if this plug-in makes the declaration in every
POM properties are derived, so can we define that property in the root POM of the multi-module reactor and it is considered for every project by the How it the path resolved by the plugin? A short example:
I assume I could set the property in the root POM e.g. to Does your plugin resolve the path always based to the POM location the property is defined? If we need to define the property with a relative directory for ever bundle's POM, there is no real benefit using Can you give me some insights, I assume I miss there something. Is there an exmaple multi-module reactor project that is using EEAs in a local directory and works for Maven builds and in the Eclipse IDE (with the respective plugin installed)? |
@maggu2810 I do not want to further pollute this ESH PR's Conversation with lastnpe.org related conversations, and will disengage from this thread. We would love to see you join the party over on https://github.com/lastnpe/ and raise issues there (or, MUCH better, pull requests - speak in code not words!). As per usual best practices, please be fine grained and have 1 actionable issue per specific topic, in the right place (e.g. https://github.com/lastnpe/eclipse-external-annotations-m2e-plugin/issues for your last point above re. the plugin, but https://github.com/lastnpe/eclipse-null-eea-augments/issues for anything related to the EEA hosted in that repo). PS: Of course, anything that's really more to do with Eclipse' JDT null analysis itself than with the lastnpe.org stuff, please instead engage over on http://bugs.eclipse.org maybe e.g. Stephan will have a look (e.g. for whatever you meant by "..the Eclipse IDE create / edit EEA support only handles.."). |
I added the external annotation classpath setting to every |
Should I close this PR as there is no interest in in-tree EEAs? |
This commit adds initial support for external annotations. There is a new project "eclipse-external-annotations" that must be imported in the Eclipse IDE. If added already some annotations: * JRE * Object.toString() returns a `@NonNull` String * Collections.empty... returns a `@NonNull` reference * Collections.singleton... returns a `@NonNull` reference * Collections.synchronized... returns a `@NonNull` reference * Collections.unmodifiable... returns a `@NonNull` reference * ... added a lot more ones while working and rebasing that branch. * slf4j * LoggerFactory.getLogger(...) returns a `@NonNull` reference If a project should use external annotations this needs to be configured for the Eclipse IDE in the project settings of each project. See [Configuring a project to use external annotations] Open the project settings and navigate to "Java Build Path", "Library" tab. Set the external annotation reference for "JRE System Library" and "Plug-in Dependencies". This has been already applied for the core and the core.things project. For the headless consumption a "dedicated path" is possible ("-annotationpath location"). See [Headless consumption] This setting has been already added to the POM file. Add new annotations is very easy as it is supported by the IDE. See [Creating external annotations] [Configuring a project to use external annotations]: http://help.eclipse.org/oxygen/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Ftasks%2Ftask-using_external_null_annotations.htm&cp=1_3_9_2&anchor=configure [Creating external annotations]: http://help.eclipse.org/oxygen/index.jsp?topic=%2Forg.eclipse.jdt.doc.user%2Ftasks%2Ftask-using_external_null_annotations.htm&cp=1_3_9_2&anchor=create [Headless consumption]: https://wiki.eclipse.org/JDT_Core/Null_Analysis/External_Annotations#Headless_consumption Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
Signed-off-by: Markus Rathgeb <maggu2810@gmail.com>
@triller-telekom Are you still on this or do you expect anybody else to follow up here? |
I think it highly depends on this discussion, i.e. what happens if we decide to turn off warnings for null annotations. If turning off the warnings makes the eea problem disappear for us, I currently do not see the need for integrating them into ESH. If the errors that we would like to keep for null annotations somehow "cannot be silenced" without those eea, then we should integrate them. |
This PR is similar to eclipse-archived#4217 but instead of creating a project that contains the annotations in a directory it sets up the IDE to read the annotations from a zip file. The reason behind this is that the IDE should be for the every day developer who will not create external annotations. Nevertheless he should benefit from them which is why they are provided in a zip file. This zip file should be hosted externally, maybe together with lastnpe.org. That is also the place to add new annotations, i.e. download the project from there, change the IDE to use this project as a directory, add your annotations and afterwards the project releases a new zip file. In this PR I created a setup task for the IDE to download the annotations file and place it in a "eea" directory in the targetplatform folder. For maven there is a new dependency to the wagon-maven-plugin which enables downloading a single file to be placed where the IDE stores the file too. The compile task uses this zip file as "annotationpath". Also I reactivated the "Unchecked conversion from non-annotated type to @nonnull type" check by setting it to warning level, otherwise (if set to ignore) external annotations are not needed. As an example I updated the ".classpath" file of the sonos binding to use the downloaded zip file. The parts which are still valid from PR eclipse-archived#4217 are: * all created external annotations * all updated .classpath files (IF adjusted to use the zip file instead of a directory) The URL where the file will be hosted is still left open, which is why this PR is WIP.
This PR is similar to eclipse-archived#4217 but instead of creating a project that contains the annotations in a directory it sets up the IDE to read the annotations from a zip file. The reason behind this is that the IDE should be for the every day developer who will not create external annotations. Nevertheless he should benefit from them which is why they are provided in a zip file. This zip file should be hosted externally, maybe together with lastnpe.org. That is also the place to add new annotations, i.e. download the project from there, change the IDE to use this project as a directory, add your annotations and afterwards the project releases a new zip file. In this PR I created a setup task for the IDE to download the annotations file and place it in a "eea" directory in the targetplatform folder. For maven there is a new dependency to the wagon-maven-plugin which enables downloading a single file to be placed where the IDE stores the file too. The compile task uses this zip file as "annotationpath". Also I reactivated the "Unchecked conversion from non-annotated type to @nonnull type" check by setting it to warning level, otherwise (if set to ignore) external annotations are not needed. As an example I updated the ".classpath" file of the sonos binding to use the downloaded zip file. The parts which are still valid from PR eclipse-archived#4217 are: * all created external annotations * all updated .classpath files (IF adjusted to use the zip file instead of a directory) The URL where the file will be hosted is still left open, which is why this PR is WIP. Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@maggu2810 You might have seen that I created a new PR that fetches the EEAs from a maven repo and puts them on the classpath from where the compiler called via maven picks them up. This works fine. I have also created a PR in the lastnpe repo to create one large jar file will all annotations to which we can refer to: lastnpe/eclipse-null-eea-augments#57 How ever I still see 2 open points here:
Your EEAs could go into the lastnpe repo so we will even save a CQ here :) |
yeah, we haven't had the need to give that much thought over on lastNPE.org - so new issue 'bout that! |
This PR is similar to eclipse-archived#4217 but instead of creating a project that contains the annotations in a directory it sets up the IDE to read the annotations from a zip file. The reason behind this is that the IDE should be for the every day developer who will not create external annotations. Nevertheless he should benefit from them which is why they are provided in a zip file. This zip file should be hosted externally, maybe together with lastnpe.org. That is also the place to add new annotations, i.e. download the project from there, change the IDE to use this project as a directory, add your annotations and afterwards the project releases a new zip file. In this PR I created a setup task for the IDE to download the annotations file and place it in a "eea" directory in the targetplatform folder. For maven there is a new dependency to the wagon-maven-plugin which enables downloading a single file to be placed where the IDE stores the file too. The compile task uses this zip file as "annotationpath". Also I reactivated the "Unchecked conversion from non-annotated type to @nonnull type" check by setting it to warning level, otherwise (if set to ignore) external annotations are not needed. As an example I updated the ".classpath" file of the sonos binding to use the downloaded zip file. The parts which are still valid from PR eclipse-archived#4217 are: * all created external annotations * all updated .classpath files (IF adjusted to use the zip file instead of a directory) The URL where the file will be hosted is still left open, which is why this PR is WIP. Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
@maggu2810 & @triller-telekom Is #4513 the successor for this PR, so that this should be closed? |
As already written above, regardless of successor or not, this PR is closed as there is no interest in in-tree EEAs. |
This PR is similar to eclipse-archived#4217 but instead of creating a project that contains the annotations in a directory it sets up the IDE to read the annotations from a zip file. The reason behind this is that the IDE should be for the every day developer who will not create external annotations. Nevertheless he should benefit from them which is why they are provided in a zip file. This zip file should be hosted externally, maybe together with lastnpe.org. That is also the place to add new annotations, i.e. download the project from there, change the IDE to use this project as a directory, add your annotations and afterwards the project releases a new zip file. In this PR I created a setup task for the IDE to download the annotations file and place it in a "eea" directory in the targetplatform folder. For maven there is a new dependency to the wagon-maven-plugin which enables downloading a single file to be placed where the IDE stores the file too. The compile task uses this zip file as "annotationpath". Also I reactivated the "Unchecked conversion from non-annotated type to @nonnull type" check by setting it to warning level, otherwise (if set to ignore) external annotations are not needed. As an example I updated the ".classpath" file of the sonos binding to use the downloaded zip file. The parts which are still valid from PR eclipse-archived#4217 are: * all created external annotations * all updated .classpath files (IF adjusted to use the zip file instead of a directory) The URL where the file will be hosted is still left open, which is why this PR is WIP. Signed-off-by: Stefan Triller <stefan.triller@telekom.de>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/how-do-you-deal-with-jdt-static-null-analysis-limitations/95036/14 |
This commit adds initial support for external annotations.
There is a new project "eclipse-external-annotations" that must be
imported in the Eclipse IDE.
If added already some annotations:
@NonNull
String@NonNull
reference@NonNull
reference@NonNull
reference@NonNull
reference@NonNull
referenceIf a project should use external annotations this needs to be configured
for the Eclipse IDE in the project settings of each project.
See Configuring a project to use external annotations
Open the project settings and navigate to "Java Build Path", "Library"
tab. Set the external annotation reference for "JRE System Library" and
"Plug-in Dependencies".
This has been already applied for the core and the core.things project.
For the headless consumption a "dedicated path" is possible
("-annotationpath location").
See Headless consumption
This setting has been already added to the POM file.
Add new annotations is very easy as it is supported by the IDE.
See Creating external annotations