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

license policy enforcement with maven project implementation. #207

Merged
merged 2 commits into from
Sep 1, 2021

Conversation

rremer
Copy link

@rremer rremer commented May 25, 2021

Sorry for the massive PR, the complexity of #203 eluded me and I just kept coding, should have circled back a few times with you first @mathieucarbou .

Some overview:

  • this code meets my immediate needs within my company, so I consider it ready to merge (sans any feedback changes you have me make)
  • everything is implemented within the same check goal, however I've setup my integration tests and primary classes to support a report goal which we can do in a separate review
  • I deviated slightly from this project's current maven-invoker-plugin approach to running integration tests. I wanted a few primary cases to be covered outside my unit tests, and the invoker method was really slow and impossible to debug, so I'm org.apache.maven.it.Verifier within junit to execute integration tests against projects in src/test/resources/config, hope that makes sense. Happy to refactor the existing integration tests if you like this method.
  • all the logic for dependency discovery is currently within MavenProjectLicenses, as I was thinking about future cases where we may invoke this maven plugin to performe license enforcement on other languages/frameworks (gradle+java, python, whatever). I haven't implemented a config-driven factory for this, as it seemed a little premature and obtuse, but that was my thinking down-the-line

And one question for you:
Would you like me to put some documentation in src/site? Or we maintain a wiki? The parameters I've added are javadoc, but some common configuration examples would likely be helpful to others who don't know to go digging in this projects test resources

@mathieucarbou
Copy link
Owner

Wow! Thanks a lot!
I will definitely have a look. I am a bit overwhelmed at the moment but I think I will be able to have a look soon. No worry for the big changes. I was expecting that. I will check. And I like the idea to have IT tests all done the same way, but I do not have a strong opinion, as long as we have some ;-) Some people prefer one way, some another way. So it is totally ok for me if we leave the code with 2 different approaches. It can only speed up people's PR if they have already something they know in place.
For the doc, we only have the README for now that holds some doc outside of what's generated. I know there is a way to add sections to the plugin documentation but it was never done before. So I will let you choose!

@mathieucarbou mathieucarbou self-requested a review May 25, 2021 21:02
@mathieucarbou mathieucarbou added this to the 4.2 milestone May 25, 2021
@mathieucarbou mathieucarbou linked an issue May 25, 2021 that may be closed by this pull request
@rremer
Copy link
Author

rremer commented Jun 28, 2021

Hey @mathieucarbou , let me know if I can ease your review in any way. Happy to schedule a hangout if you want a walkthrough.

@mathieucarbou
Copy link
Owner

Hey @mathieucarbou , let me know if I can ease your review in any way. Happy to schedule a hangout if you want a walkthrough.

Hello,

Thanks but not necessary at the moment: the delay is caused because after 20 yeas in Canada we are relocating in France with the whole family and the months of May, June and July are crazy because of everything an international move implies during Covid.

Sadly I don't have any good solution to propose to speed up because I am the only one actively working on the project (and who could review).

If you need something in Maven central fast tough, what could be done is a feature branch from where we release a beta, like 4.2.beta1

@rremer
Copy link
Author

rremer commented Jun 28, 2021

No need for a release (but thanks for the offer!), I just didn't want to forget about this... context switching after a month+ is a little harder.

I hope your move goes smoothly and you can enjoy a nice bottle of the local vintage before jumping back into this project ;)

@mathieucarbou
Copy link
Owner

No need for a release (but thanks for the offer!), I just didn't want to forget about this... context switching after a month+ is a little harder.

I hope your move goes smoothly and you can enjoy a nice bottle of the local vintage before jumping back into this project ;)

Ahah!! Thanks a lot!
I will be disrupted in July too but here in Canada we have no help for the kids so I don't have any spare time.
In July we will be in the family so I will have more time. I have a list of items already planned including this review plus a couple of issues, no worry ;-)

@mathieucarbou mathieucarbou merged commit 8f35589 into mathieucarbou:master Sep 1, 2021
@mathieucarbou
Copy link
Owner

@rremer : I have merged - would it be possible for you to add a little doc section in the README with a little example ? Not an exhaustive section but enough so that people can easily see that the plugin is able to also check for license compat' ;-)
Thanks!

@rremer
Copy link
Author

rremer commented Sep 1, 2021

Thanks @mathieucarbou ! Two PRs for you:
#219 - can be used standalone
mathieucarbou/parent-pom#6 - would enhance the above PR if we bump the version of the parent pom in license-maven-plugin and then generate the plugin documentation as a maven site, published to github pages.

@rremer
Copy link
Author

rremer commented Oct 29, 2021

FYI @mathieucarbou I've been using 4.2.rc1 at my company since the day after you released it, and have found no regressions in our previous enforcement. A few things have come up regarding the new license enforcement feature, which I'd like to document here for posterity. Maybe some of this is relevant for a FAQ or elsewhere.

If an artifact has multiple licenses, some which are allowed and some which are dissallowed, shouldn't we dissallow that artifact?

Actually no. I have a test for this specific case, and double checked the maven model reference which confirms that multiple licenses declared in your pom is supposed to imply a choice as to which license you would like to use. This seems to be reflected in the wild as well, where I've seen jars released under multiple licenses with a combinatory name, like "CDDL + GPLv2 with classpath exception" for javax.annotation-api

Sometimes the same license is referenced multiple times because of differences in protocol.

This is true, and it's actually somewhat worse than that due to inconsistencies in poms declaring url or not. As an aggregious example, the following all refer to the same license but is required on my team to allow Apache 2.0:

<dependencyPolicy>
    <type>LICENSE_NAME</type>
    <rule>APPROVE</rule>
    <value>Apache Software License - Version 2.0</value>
</dependencyPolicy>
<dependencyPolicy>
    <type>LICENSE_NAME</type>
    <rule>APPROVE</rule>
    <value>Apache License, Version 2.0</value>
</dependencyPolicy>
<dependencyPolicy>
    <type>LICENSE_URL</type>
    <rule>APPROVE</rule>
    <value>http://www.apache.org/licenses/LICENSE-2.0.html</value>
</dependencyPolicy>
<dependencyPolicy>
    <type>LICENSE_URL</type>
    <rule>APPROVE</rule>
    <value>http://www.apache.org/licenses/LICENSE-2.0.txt</value>
</dependencyPolicy>
<dependencyPolicy>
    <type>LICENSE_NAME</type>
    <rule>APPROVE</rule>
    <value>The Apache Software License, Version 2.0</value>
</dependencyPolicy>
<dependencyPolicy>
    <type>LICENSE_URL</type>
    <rule>APPROVE</rule>
    <value>https://www.apache.org/licenses/LICENSE-2.0.txt</value>
</dependencyPolicy>

I'll muse on this. I don't think I want to add regex capability, that could weaken enforcement due to bad regex. That said, loosening the URL specifically on protocol I'd be cool with (these are literal URL objects, so stripping protocol is free and wouldn't weaken enforcement imo). The other thought I had is more robust, but more complicated, which is to:

  • support configuring the plugin via a resource jar containing the literal text body of licenses to be allowed
  • supporting a lookup mechanism of license text in the dependencies, like a cached pull of the url, or a load of LICENSE or LICENSE.txt from the dependent jarfile
  • enforcement on LICENSE_CONTENT

@mathieucarbou
Copy link
Owner

Thanks! This is all good news! I was waiting some weeks before doing a final release so I think I'll be able to do that in November.

Feel free to add a section in the readme or an isolated mardown faq file perhaps, I will merge. The doc is now generated directly from main automatically with a GitHub action.

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.

design discussion: goal for per-dependency license enforcement
2 participants