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

Update Maven Enforcer, add Animal Sniffer and Test annotations #14

Merged
merged 4 commits into from Aug 21, 2017

Conversation

@oleg-nenashev
Copy link
Member

commented Aug 8, 2017

The most of the configurations come from Plugin POM. In longer term we need to merge two POMs somehow (e.g. make this POM a parent for Plugin POM), but now I just moved the common Enforcer and Animal Sniffer checks

Downstream PRs:

@reviewbybees esp. @jglick since it enforces upper bounds in Jenkins core && may collide with jenkinsci/jenkins#2956

Update Maven Enforcer, add Animal Sniffer and Test annotations
The most of the configurations come from Plugin POM. In longer term we need to merge two POMs somehow (e.g. make this POM a parent for Plugin POM), but now I just moved the common Enforcer and Animal Sniffer checks
@reviewbybees

This comment has been minimized.

Copy link

commented Aug 8, 2017

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2017

Hmm... @olamy @aheritier Is is as designed In Apache? Meven Embedded 3.5.0 bundles Maven Core 3.3.1

+-org.jenkins-ci.main.maven:maven35-agent:1.12-SNAPSHOT
  +-org.apache.maven:maven-embedder:3.5.0
    +-org.apache.maven:maven-core:3.3.1

+-org.jenkins-ci.main.maven:maven35-agent:1.12-SNAPSHOT
  +-org.apache.maven:maven-embedder:3.5.0
    +-org.apache.maven:maven-compat:3.5.0
      +-org.apache.maven:maven-core:3.5.0
@aheritier

This comment has been minimized.

Copy link
Member

commented Aug 8, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2017

@aheritier Yes, my guess, it's a bug in Apache Maven Embedder dependencies. In jenkinsci/maven-interceptors#12 I made the things working, but I will definitely raise a defect to the MEMBEDDER

@oleg-nenashev oleg-nenashev requested a review from jglick Aug 8, 2017

@jglick
Copy link
Member

left a comment

May be OK but really it needs to be preintegrated with more downstream POMs to see how well it works.

pom.xml Outdated
<!-- findbugs dep managed to provided and optional so is not shipped and missing annotations ok -->
<exclude>com.google.code.findbugs:annotations</exclude>
</excludes>
<!-- To add exclusions in a Jenkins plugin, use:

This comment has been minimized.

Copy link
@jglick

jglick Aug 8, 2017

Member

component, generally not a Jenkins plugin

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 15, 2017

Author Member

Agreed, will patch

pom.xml Outdated
-->
<requireReleaseDeps>
<message>No Snapshots Allowed For Release Versions</message>
<onlyWhenRelease>true</onlyWhenRelease>

This comment has been minimized.

Copy link
@jglick

jglick Aug 8, 2017

Member

Note that maven-release-plugin enforces this on its own anyway.

pom.xml Outdated

<!-- TODO: Disabled. Ideally it should be enforced for ANY library bundled into the Jenkins core,
But right now there no good way to determine whether it is bundled or not.
It should be done via profles somehow

This comment has been minimized.

Copy link
@jglick

jglick Aug 8, 2017

Member

I do not see any need for profiles for this.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 15, 2017

Author Member

What would be your proposal?

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

Any module, or intermediate parent POM, which requires bannedDependencies can add it on its own. Anyway

  • the list of excludes is unlikely to be sharable across classes of modules (it makes sense in plugin-pom)
  • a module POM cannot inherit a profile from its parent in a way that would be useful here (since you cannot activate it automatically; longstanding Maven limitation)

Just delete the commented-out block IMO.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2017

Author Member

We could add another parent POM for in-Jenkins components. Everybody likes Russian dolls, right? :/

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2017

Author Member

Created https://issues.jenkins-ci.org/browse/JENKINS-46237 for it in order to follow-up later

@jglick
Copy link
Member

left a comment

Some minor complaints. More broadly, this needs downstream PRs demonstrating the effect on the most important consumers: jenkinsci/jenkins/pom.xml, plugin-pom/pom.xml, and some representative plugin using plugin-pom. Presumably the direct downstream PRs should be able to delete a bunch of now-redundant configuration.

pom.xml Outdated
</dependency>
</dependencies>
</dependencyManagement>
<dependencies>

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

Never add any dependencies to a parent POM unless (like plugin-pom) it is designed solely for a specific packaging and class of module. A module using this parent may add the dependency if it wants it.

pom.xml Outdated
<groupId>org.codehaus.mojo</groupId>
<artifactId>animal-sniffer-annotations</artifactId>
<scope>provided</scope>
<optional>true</optional><!-- no need to have this at runtime -->

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

Is there a purpose in being both provided and optional? I see that plugin-pom does this but I wonder if it is a mistake. @stephenc would know best.

This comment has been minimized.

Copy link
@aheritier

aheritier Aug 16, 2017

Member

provided should be enough but I have a doubt. Maybe a hack used in others cases (maybe with the maven-hpi-plugin)

pom.xml Outdated
<dependency>
<groupId>org.jenkins-ci</groupId>
<artifactId>test-annotations</artifactId>
<version>${test-annotations.version}</version>

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

This would be a natural candidate for dependencyManagement.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2017

Author Member

No strong opinion. It seems to be useful to have it in all projects inheriting from this POM. In plugin POM we ship multiple deps in such way as well.

This comment has been minimized.

Copy link
@aheritier

aheritier Aug 16, 2017

Member

I agree with @jglick advice to never add a dep in such high level pom because you cannot remove them in children. This is a mandatory rule for compile deps. For deps and provided like here it could be an exception I think.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2017

Author Member

ok

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2017

Author Member

Note that plugin POM already adds a test dependency for Test Annotations: https://github.com/jenkinsci/plugin-pom/blob/master/pom.xml#L244-L247

I will move it to the dependency management for now

pom.xml Outdated
<version>(,3.0),[3.0.4,)</version>
<message>Build with Maven 3.0.4 or later. Maven 3.0 through 3.0.3 inclusive do not pass correct settings.xml to Maven Release Plugin.</message>
<version>[3.1.0,)</version>
<message>3.1.0 required at least.</message>

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

3.1.0 is pretty old; feel free to go older. (What do we actually test with?)

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Aug 15, 2017

Member

3.1.0 is pretty old; feel free to go older

Did you mean to write this?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2017

Author Member

By default the build on my machine runs with 3.3.3.
Requesting 3.3.9 or above could be reasonable.

This comment has been minimized.

Copy link
@aheritier

aheritier Aug 16, 2017

Member

yes let's start at 3.3.9 nowadays.

pom.xml Outdated

<!-- TODO: Disabled. Ideally it should be enforced for ANY library bundled into the Jenkins core,
But right now there no good way to determine whether it is bundled or not.
It should be done via profles somehow

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

Any module, or intermediate parent POM, which requires bannedDependencies can add it on its own. Anyway

  • the list of excludes is unlikely to be sharable across classes of modules (it makes sense in plugin-pom)
  • a module POM cannot inherit a profile from its parent in a way that would be useful here (since you cannot activate it automatically; longstanding Maven limitation)

Just delete the commented-out block IMO.

pom.xml Outdated
<dependency>
<groupId>org.codehaus.mojo</groupId>
<artifactId>extra-enforcer-rules</artifactId>
<version>1.0-beta-4</version>

This comment has been minimized.

Copy link
@jglick

jglick Aug 15, 2017

Member

BTW 1.0-beta-6 is available. But beta 4 has the critical NPE fix.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2017

jenkinsci/jenkins/pom.xml, plugin-pom/pom.xml, and some representative plugin using plugin-pom.

Plugin POM does not inherit from this POM. As I said in the PR description, it should be done at some point, but now these POMs are independent (which is weird)

@aheritier
Copy link
Member

left a comment

Few comments, but LGTM otherwise

pom.xml Outdated
<groupId>org.codehaus.mojo</groupId>
<artifactId>animal-sniffer-annotations</artifactId>
<scope>provided</scope>
<optional>true</optional><!-- no need to have this at runtime -->

This comment has been minimized.

Copy link
@aheritier

aheritier Aug 16, 2017

Member

provided should be enough but I have a doubt. Maybe a hack used in others cases (maybe with the maven-hpi-plugin)

pom.xml Outdated
<dependency>
<groupId>org.jenkins-ci</groupId>
<artifactId>test-annotations</artifactId>
<version>${test-annotations.version}</version>

This comment has been minimized.

Copy link
@aheritier

aheritier Aug 16, 2017

Member

I agree with @jglick advice to never add a dep in such high level pom because you cannot remove them in children. This is a mandatory rule for compile deps. For deps and provided like here it could be an exception I think.

pom.xml Outdated
<version>(,3.0),[3.0.4,)</version>
<message>Build with Maven 3.0.4 or later. Maven 3.0 through 3.0.3 inclusive do not pass correct settings.xml to Maven Release Plugin.</message>
<version>[3.1.0,)</version>
<message>3.1.0 required at least.</message>

This comment has been minimized.

Copy link
@aheritier

aheritier Aug 16, 2017

Member

yes let's start at 3.3.9 nowadays.

@oleg-nenashev

This comment has been minimized.

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Aug 16, 2017

@jglick @aheritier Created pull requests to Jenkins core and Remoting as a reference. The work on my machine (c).

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 17, 2017

Plugin POM does not inherit from this POM. As I said in the PR description, it should be done at some point, but now these POMs are independent

But should not the switch to have plugin-pom inherit from jenkins-pom be a goal of this work?

@jglick
Copy link
Member

left a comment

animal-sniffer-annotations should also be moved to dependencyManagement I think. Other than that this looks OK in and of itself (have not yet looked at the new downstreams).

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

@jglick Done. I would rather vote for having common dependencies declared in Maven POM, but maybe I should google for "Thinking in Maven" first

@jglick
jglick approved these changes Aug 21, 2017
@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2017

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented Aug 21, 2017

I am about landing and releasing it to unblock the Maven plugin patches. Will do it on the evening if there is no -1s

@aheritier
Copy link
Member

left a comment

🐝

@oleg-nenashev oleg-nenashev merged commit 1e85177 into jenkinsci:master Aug 21, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details
<version>(,3.0),[3.0.4,)</version>
<message>Build with Maven 3.0.4 or later. Maven 3.0 through 3.0.3 inclusive do not pass correct settings.xml to Maven Release Plugin.</message>
<version>[3.3.9,)</version>
<message>3.3.9 is required at least.</message>

This comment has been minimized.

Copy link
@jglick

jglick Sep 7, 2017

Member

BTW was there any specific reason for this bump, or just to be on the safe side?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Sep 7, 2017

Author Member

See the discission in #14 (comment) . No specific reason for this version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.