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 a maven plugin for the dash license tool #44

Merged
merged 2 commits into from Dec 31, 2020

Conversation

mbooth101
Copy link
Member

This PR contains two changes:

  1. Refactor into multimodule project to keep maven-specific code out of the core code
  2. Add a maven plugin as a separate module

The maven plugin is fully configurable and has feature parity with the command line tool. Also allows maven-dependency-plugin style artifact filtering which allows users to ignore deps that are not interesting (e.g. test deps or deps from the Eclipse Foundation
itself.)

Still to do:

  1. add some integration tests
  2. deps have hard-coded origin of either "orbit" or "mavencentral" which is obviously not true, but command line tool makes the same assumption and I wanted to (at least initially) match the behaviour of the command line tool
  3. license tool seems to have no knowledge of the classifier component of maven coordinates, so it's probable we are missing stuff -- probably need to enhance IContentId in the core module
  4. delete maven-specific parsing code from core module/command line tool

Invoking the plugin:

For a trivial, fully-compliant project you might get output like this:

$ mvn org.eclipse.dash:dash-maven-plugin:license-check -Ddash.summary=DEPENDENCIES
[INFO] Scanning for projects...
[INFO] 
[INFO] -------------< org.eclipse.dash:org.eclipse.dash.licenses >-------------
[INFO] Building org.eclipse.dash.licenses 0.0.1-SNAPSHOT
[INFO] --------------------------------[ jar ]---------------------------------
[INFO] 
[INFO] --- dash-maven-plugin:0.0.1-SNAPSHOT:license-check (default-cli) @ org.eclipse.dash.licenses ---
[INFO] Vetted license information was found for all content. No further investigation is required.
[INFO] Summary file was written to: /home/mbooth/workspace-dash/dash-licenses/dash-core/DEPENDENCIES
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  2.521 s
[INFO] Finished at: 2020-12-18T12:03:58Z
[INFO] ------------------------------------------------------------------------

$ cat DEPENDENCIES 
maven/mavencentral/org.glassfish/jakarta.json/2.0.0, EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0, approved, emo_ip_team
maven/mavencentral/org.checkerframework/checker-compat-qual/2.5.3, MIT, approved, clearlydefined
maven/mavencentral/commons-cli/commons-cli/1.4, Apache-2.0, approved, CQ13132
maven/mavencentral/org.apache.commons/commons-csv/1.8, Apache-2.0, approved, clearlydefined
maven/mavencentral/com.google.flogger/flogger/0.5.1, Apache-2.0, approved, clearlydefined

For a more complex multi-module project like tycho itself:

$ mvn org.eclipse.dash:dash-maven-plugin:license-check 
........  lots of maven output .......
[INFO] Tycho Target Platform Validation Plugin                   [maven-plugin]                                                          
[INFO] Tycho POM-less build extension                                     [jar]                                                          
[INFO] Tycho Dependency Tools Plugin                             [maven-plugin]                                                          
[INFO]                                                              
[INFO] ----------------------< org.eclipse.tycho:tycho >-----------------------                                                          
[INFO] Building Tycho 2.1.0                                              [1/72]                                                          
[INFO] --------------------------------[ pom ]---------------------------------                                                          
[INFO]                                                              
[INFO] --- dash-maven-plugin:0.0.1-SNAPSHOT:license-check (default-cli) @ tycho ---                                                      
[INFO] License information could not be automatically verified for the following content:                                                
[INFO]                                                              
[INFO] maven/mavencentral/classworlds/classworlds/1.1-alpha-2      
[INFO] maven/mavencentral/com.google.code.findbugs/jsr305/3.0.2    
[INFO] maven/mavencentral/org.codehaus.mojo/animal-sniffer-annotations/1.14                                                              
[INFO] maven/mavencentral/org.fedoraproject.p2/org.fedoraproject.p2/0.0.1-SNAPSHOT                                                       
[INFO] p2/orbit/p2.eclipse-plugin/biz.aQute.bndlib/5.2.0.202010142003                                                                    
[INFO] p2/orbit/p2.eclipse-plugin/org.fedoraproject.p2/0.0.1.202012181131                                                                
[INFO]                                                              
[INFO] This content is either not correctly mapped by the system, or requires review.                                                    
[INFO]                                                              
[INFO]                                                              
[INFO] Summary file was written to: /home/mbooth/workspace-tycho/org.eclipse.tycho/target/dash/summary                                   
[INFO] Review request file was written to: /home/mbooth/workspace-tycho/org.eclipse.tycho/target/dash/review                             
[INFO] ------------------------------------------------------------------------                                                          
[INFO] Reactor Summary for Tycho 2.1.0:                            
[INFO]                                                              
[INFO] Tycho .............................................. SUCCESS [  8.319 s]                                                          
[INFO] Tycho OSGi Bundles Parent .......................... SKIPPED
[INFO] Tycho OSGi Bundles Target Platform ................. SKIPPED
[INFO] Tycho Noop Equinox password provider ............... SKIPPED
........  lots more maven output .......

Because this project is not fully compliant, you also get a review file this time:

$ cat /home/mbooth/workspace-tycho/org.eclipse.tycho/target/dash/review 
The following content requires review:

* [ ] maven/mavencentral/classworlds/classworlds/1.1-alpha-2
  - [clearlydefined](https://clearlydefined.io/definitions/maven/mavencentral/classworlds/classworlds/1.1-alpha-2) OTHER (45)
  - [Search IPZilla](https://dev.eclipse.org/ipzilla/buglist.cgi?short_desc_type=anywords&short_desc=classworlds&long_desc_type=substring)
  - [Maven Central](https://search.maven.org/artifact/classworlds/classworlds/1.1-alpha-2/jar)
..... blah blah blah .....

Signed-off-by: Mat Booth <mat.booth@redhat.com>
The maven plugin is fully configurable and has feature parity with
the command line tool. Also allows maven-dependency-plugin style
artifact filtering which allows users to ignore deps that are not
interesting (e.g. test deps or deps from the Eclipse Foundation
itself.)

Fixes eclipse#7

Signed-off-by: Mat Booth <mat.booth@redhat.com>
@waynebeaton
Copy link
Member

This is awesome, Matt. I'll try to review it over the weekend.

@waynebeaton
Copy link
Member

deps have hard-coded origin of either "orbit" or "mavencentral" which is obviously not true, but command line tool makes the same assumption and I wanted to (at least initially) match the behaviour of the command line tool

We do the best that we can with the information available (that is, none). Given that Maven knows the identity of the repositories that it's pulling from, you're right that we should be able to do better. Note that there are some larger questions that we'll have to address with regard to ClearlyDefined. Can we, for example, assume that the same GAV coming from Maven Central and Maven Repository represent the exact same content? I'm thinking that the answer to this question is probably yes, but we'll have to do some due diligence to confirm (FWIW, we're basically making this assumption now by guessing "mavencentral").

license tool seems to have no knowledge of the classifier component of maven coordinates, so it's probable we are missing stuff -- probably need to enhance IContentId in the core module

This is actually a limitation of ClearlyDefined. I've been meaning to open an issue with them to sort out how to best deal with this information. My assumption is that they'll consider it part of the name. Whatever we decide to do, we'll need to coordinate with them if we're to be able to leverage their data set.

delete maven-specific parsing code from core module/command line tool

Bear in mind that we need to support non-Maven use cases as well. I'd also rather not assume that all Maven builds will necessarily use the Maven plugin.

@waynebeaton
Copy link
Member

waynebeaton commented Dec 31, 2020

The names are wrong. "Dash" is the name of the Eclipse open source project that owns this code; the tool itself is just called "License Tool". So, the artifactid should be something like "license-tool-core" as in "org.eclipse.dash:license-tool-core:0.1.0" and "org.eclipse.dash:license-tool-plugin:0.1.0" (I don't love having "maven" or "plugin" in the artifactid; is there a convention that we can adopt?)

@waynebeaton
Copy link
Member

I'm going to merge the pull request, but follow it up immediately with a commit that changes the "dash-core" and "dash-maven-plugin" directory names and tweaks other project metadata.

@waynebeaton
Copy link
Member

The classworlds hit was bogus, so I fixed it in the backend data.

It's flagging two other libraries as "restricted". I'm confident that both are actually okay, and will investigate why they're being flagged by the backend.

@waynebeaton waynebeaton merged commit dd8d6af into eclipse:master Dec 31, 2020
@mbooth101
Copy link
Member Author

mbooth101 commented Jan 4, 2021

@waynebeaton Sorry I was AFK for more than 2 weeks and missed these questions!

deps have hard-coded origin of either "orbit" or "mavencentral" which is obviously not true, but command line tool makes the same assumption and I wanted to (at least initially) match the behaviour of the command line tool

We do the best that we can with the information available (that is, none). Given that Maven knows the identity of the repositories that it's pulling from, you're right that we should be able to do better. Note that there are some larger questions that we'll have to address with regard to ClearlyDefined. Can we, for example, assume that the same GAV coming from Maven Central and Maven Repository represent the exact same content? I'm thinking that the answer to this question is probably yes, but we'll have to do some due diligence to confirm (FWIW, we're basically making this assumption now by guessing "mavencentral").

The answer is almost certainly "no" -- There are other maven repositories that are not Maven Central and there are projects that publish artifacts to those repos instead of Maven Central. The biggest other repo is probably JFrog/Bintray and if a project has this repo configured, then Dash will incorrectly say that all dep artifacts are from Maven Central when the artifact may not be found in Maven Central at all!

license tool seems to have no knowledge of the classifier component of maven coordinates, so it's probable we are missing stuff -- probably need to enhance IContentId in the core module

This is actually a limitation of ClearlyDefined. I've been meaning to open an issue with them to sort out how to best deal with this information. My assumption is that they'll consider it part of the name. Whatever we decide to do, we'll need to coordinate with them if we're to be able to leverage their data set.

delete maven-specific parsing code from core module/command line tool

Bear in mind that we need to support non-Maven use cases as well. I'd also rather not assume that all Maven builds will necessarily use the Maven plugin.

I don't suggest removing non-Maven use-cases, but I'd argue maven-based projects should always use the maven plugin -- if only because avoiding parsing maven coords ourselves is likely to be much more reliable in the long term.

The names are wrong. "Dash" is the name of the Eclipse open source project that owns this code; the tool itself is just called "License Tool". So, the artifactid should be something like "license-tool-core" as in "org.eclipse.dash:license-tool-core:0.1.0" and "org.eclipse.dash:license-tool-plugin:0.1.0" (I don't love having "maven" or "plugin" in the artifactid; is there a convention that we can adopt?)

Sure, I just made choices :-) The de-facto convention for naming maven plugins is that plugins supplied by the Apache Maven project have artifactIds of the form maven-<function>-plugin and maven plugins from third-parties like us have artifactIds of the form <function>-maven-plugin -- but is by no means a hard and fast rule, I have no strong opinion on naming.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants