Maven 1
  1. Maven 1
  2. MAVEN-518

plugins guess filename from 'dependency', should use 'artifact'

    Details

    • Type: Bug Bug
    • Status: Closed Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.0-rc2
    • Component/s: None
    • Labels:
      None
    • Number of attachments :
      0

      Description

      Quite a few plugins are mistakenly referring to and looping over dependencies when they mean to use artifacts. For example this code in the war plugin:

      <j:forEach var="dep" items="$

      {pom.dependencies}

      ">
      <j:if test="$

      {dep.getProperty('war.bundle.jar')=='true'}">
      <lib dir="${maven.repo.local}/${dep.artifactDirectory}/jars/">
      <include name="${dep.artifact}"/>
      </lib>
      </j:if>
      </j:forEach>

      Will not work if the 'jar' element has been used to override the filename of a dependency. In maven Artifacts know their filename, Dependencies don't. You can navigate from an Artifact to a Dependency if required, so that one fix might look like:

      <j:forEach var="lib" items="${pom.artifacts}">
      <j:set var="dep" value="${lib.dependency}"/>
      <j:if test="${dep.getProperty('war.bundle.jar')=='true'}

      ">
      <lib dir="$

      {maven.repo.local}

      ">
      <include name="$

      {lib.path}

      "/>
      </lib>
      </j:if>
      </j:forEach>

      While this removes all knowledge of the internal structure of the local repo from the plugin, it is still not ideal. If a project is allowed to depend on jars that are outside the local repository via the jar override mechanism then plugins should not use 'maven.repo.local' either.

      To find these issues in the plugins, go to the plugins-build directory and do:
      $ find . -name '*.jelly' | xargs grep pom.dependencies
      (NB the gump and release plugins are listed by this but are not affected)
      also do:
      $ find . -name '*.jelly' | xargs grep maven.repo.local
      the second set will pick out most of the first set of problems, but will also find the occasional case where an individual dependency is misused.

      The following files are known to be affected by this bug:
      ./ant/plugin.jelly
      ./deploy/plugin.jelly
      ./ear/plugin.jelly
      ./ejb/plugin.jelly
      ./idea/plugin.jelly
      ./jboss/plugin.jelly
      ./jdeveloper/plugin.jelly
      ./jnlp/plugin.jelly
      ./uberjar/plugin.jelly
      ./war/plugin.jelly

      (Patch to follow)

        Activity

        Hide
        dion gillard added a comment -

        It depends what you use the for, but dep.artifactPath is the culprit from memory

        Show
        dion gillard added a comment - It depends what you use the for, but dep.artifactPath is the culprit from memory
        Hide
        Brian Ewins added a comment -

        (reply to Vincent) yes, I'm sure this is a bug, in most cases. If you look at how $

        {pom.dependencies}

        is used, typically it is in a loop where the plugin attempts to copy the dependency with a filename like so:

        $

        {maven.repo.local}/{$dep.artifactDirectory}/jars/${dep.artifact}

        This is a typical example. It makes the assumption that the dependency is a jar (an obvious bug); it also assumes it knows the structure of the local repository (which it doesn't - thats set up in Artifact.generatePath currently). Finally, it is wrong because of the jar override mechanism. The jar override explicitly sets a path to an artifact, and is unrelated to the path that you'd get by looking at the dependency (this is done in ArtifactListBuilder, so there is no trace in pom.dependencies that it has been done - but the artifacts in pom.artifacts are changed).

        As Michal commented in the thread about this, it would be best if we could get something which allowed this usage:

        ${localartifact.file}

        However, since artifacts have no knowledge of where maven.repo.local is, you still need to do:

        ${maven.repo.local}

        /$

        {artifact.path}
        • unless someone makes a change to the core. Since we still have the local repo mentioned, we've not got away from the jar override problem, but it does clear up the other issues.
        Show
        Brian Ewins added a comment - (reply to Vincent) yes, I'm sure this is a bug, in most cases. If you look at how $ {pom.dependencies} is used, typically it is in a loop where the plugin attempts to copy the dependency with a filename like so: $ {maven.repo.local}/{$dep.artifactDirectory}/jars/${dep.artifact} This is a typical example. It makes the assumption that the dependency is a jar (an obvious bug); it also assumes it knows the structure of the local repository (which it doesn't - thats set up in Artifact.generatePath currently). Finally, it is wrong because of the jar override mechanism. The jar override explicitly sets a path to an artifact, and is unrelated to the path that you'd get by looking at the dependency (this is done in ArtifactListBuilder, so there is no trace in pom.dependencies that it has been done - but the artifacts in pom.artifacts are changed). As Michal commented in the thread about this, it would be best if we could get something which allowed this usage: ${localartifact.file} However, since artifacts have no knowledge of where maven.repo.local is, you still need to do: ${maven.repo.local} /$ {artifact.path} unless someone makes a change to the core. Since we still have the local repo mentioned, we've not got away from the jar override problem, but it does clear up the other issues.
        Hide
        Brett Porter added a comment -

        I think these are all fixed, but let's confirm and close it off for 1.0

        Show
        Brett Porter added a comment - I think these are all fixed, but let's confirm and close it off for 1.0

          People

          • Assignee:
            Brett Porter
            Reporter:
            Brian Ewins
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: