Quantcast

[GitHub] maven pull request: MNG-5899 Reactor should use reduced dependency...

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
12 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven pull request: MNG-5899 Reactor should use reduced dependency...

Tunaki
GitHub user trask opened a pull request:

    https://github.com/apache/maven/pull/69

    MNG-5899 Reactor should use reduced dependency pom

   

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/trask/maven MNG-5899

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/maven/pull/69.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #69
   
----
commit 1ca0deec41814815d82033645ca00fe6bbfdd813
Author: Trask Stalnaker <[hidden email]>
Date:   2015-10-01T22:09:41Z

    MNG-5899 Reactor should use reduced dependency pom

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven pull request: MNG-5899 Reactor should use reduced dependency...

Tunaki
Github user ifedorenko commented on the pull request:

    https://github.com/apache/maven/pull/69#issuecomment-145722376
 
    -1
   
    The old behaviour allowed inconsistency between dependencies used to calculate project build order and project dependencies used during the build. It also resulted in reparsing reactor project pom.xml files multiple times during the build, which affected build performance for larger projects.
   



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven pull request: MNG-5899 Reactor should use reduced dependency...

Tunaki
In reply to this post by Tunaki
Github user trask commented on the pull request:

    https://github.com/apache/maven/pull/69#issuecomment-145723525
 
    @ifedorenko, thanks for reviewing this.  Any thoughts what we should do with the downstream implication of this change?  https://issues.apache.org/jira/browse/MSHADE-206



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven pull request: MNG-5899 Reactor should use reduced dependency...

Tunaki
In reply to this post by Tunaki
Github user ifedorenko commented on the pull request:

    https://github.com/apache/maven/pull/69#issuecomment-145727506
 
    I am not sure I fully understand the problem, but maven generally expects project dependencies to stay the same during the build. If you need to suppress certain storm-core dependencies from "leaking" into uber-jar projects, I think you should be able to simple mark those dependencies as optional=true (which really means "non-transitive").


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven pull request: MNG-5899 Reactor should use reduced dependency...

Tunaki
In reply to this post by Tunaki
Github user trask commented on the pull request:

    https://github.com/apache/maven/pull/69#issuecomment-145730242
 
    The nice thing about how maven-shade-plugin's "dependency reduced pom" used to work, is that you could run mvn compile or mvn test from your parent module (and so maven shade plugin wouldn't kick in since it is bound to package phase), and transitive dependencies would work as normal.  And you could run mvn package or mvn install from your parent module, and maven shade plugin would kick in and expose the shaded jar and dependency reduced pom to downstream modules.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven pull request: MNG-5899 Reactor should use reduced dependency...

Tunaki
In reply to this post by Tunaki
Github user revans2 commented on the pull request:

    https://github.com/apache/maven/pull/69#issuecomment-145854660
 
    @ifedorenko Without this kind of functionality using the shade plugin with multi-module projects is a huge pain, and would require us to use an external build system to coordinate the maven builds.  You have to fully build/install each shaded package independent of all other packages that depend on it.
   
    I understand that it is ugly to have the dependencies change as the build progresses.  I agree it really confuses things to run dependency:tree and see it be completely different from what is actually pulled in.  But it is not as bad as getting the wrong uber-jar that is missing things it needs.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven pull request: MNG-5899 Reactor should use reduced dependency...

Tunaki
In reply to this post by Tunaki
Github user aqueance commented on the pull request:

    https://github.com/apache/maven/pull/69#issuecomment-216595519
 
    In addition to the problems mentioned above, that [hack](https://github.com/apache/maven/blame/3533599e42a4a563abca33a69ed0f34c19815479/maven-aether-provider/src/main/java/org/apache/maven/repository/internal/DefaultArtifactDescriptorReader.java#L287) also breaks the transitive dependency collection functionality in general, because it employs a [Reactor-wide](https://github.com/apache/maven/blame/3533599e42a4a563abca33a69ed0f34c19815479/maven-core/src/main/java/org/apache/maven/ReactorReader.java#L69) [cache](https://github.com/apache/maven/blame/3533599e42a4a563abca33a69ed0f34c19815479/maven-core/src/main/java/org/apache/maven/ReactorReader.java#L145) of parsed POMs that fails to take into account the `RepositorySystemSession` contexts in which the cached POMs were parsed.
   
    A cache with a broken key scheme is not a cache, it's a source of unexpected behavior: if any `RepositorySystemSession` is at play other than the one used by the Reactor, collecting transitive dependencies now results in a dependency tree that is **practically random**, as it depends on:
   
      * what POMs have already been parsed by the Reactor during the build (this is why splitting the build into multiple phases works around the problems caused by this bug),
      * what other `RepositorySystemSession` instances have been used to parse and cache POMs so far,
      * what command line arguments were used to invoke Maven.
   
    I have a Maven plugin that computes transitive dependencies of the host project by cloning the `RepositorySystemSession` used by the reactor to set properties that activate build profiles, which in turn contain dependencies to be taken into account when computing a custom dependency tree.
   
    The API to do so is [public](http://wiki.eclipse.org/Aether) so one expects the functionality work, which it did until Maven 3.3 came along. Now because of that bug, the functionality stopped working reliably.
   
    Speed is important, sure, but correct functionality is probably even more so. So please remove that hack, and if its function is indeed necessary, then it needs to be replaced with a proper cache, one that takes the active `RepositorySystemSession` into account.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven issue #69: MNG-5899 Reactor should use reduced dependency pom

Tunaki
In reply to this post by Tunaki
Github user madisparn commented on the issue:

    https://github.com/apache/maven/pull/69
 
    Why the hack removal has not been merged yet?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven issue #69: MNG-5899 Reactor should use reduced dependency pom

Tunaki
In reply to this post by Tunaki
Github user ifedorenko commented on the issue:

    https://github.com/apache/maven/pull/69
 
    I still vote -1 on this change.
   
    While I appreciate "dependency reduced pom" usecase, I'd like the following two concerns addressed first:
   
    * Mutated pom.xml files must not invalidate original reactor ProjectDependencyGraph. More specifically, if the original graph allowed certain build order, the new graph must still allow the same order. In practice this means the new graph must not have any new dependencies, which is rather tricky to guarantee when we consider dependency `<excludes>` and dependency management.
   
    * Implementation must scale well to 5K+ modules and 5K managed external dependencies. In practice this requires some sort of caching to avoid repeated reparsing/reinterpollation of reactor pom.xml files.
   
    Additionally, I'd like to understand expected behaviour when projects with dependency reduced pom files are excluded from the build using `--projects` command line argument or pom-reducing mojo is not part of selected build phase. I have not analyzed this in details, but I believe mutable pom.xml files can lead to odd/unexpected build results that will be difficult to explain and hence require extra support effort (I happen to help maintain builds for a  reasonably large developer community, so supportability is an important concern for me).



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven issue #69: MNG-5899 Reactor should use reduced dependency pom

Tunaki
In reply to this post by Tunaki
Github user ndimiduk commented on the issue:

    https://github.com/apache/maven/pull/69
 
    > Implementation must scale well to 5K+ modules and 5K managed external dependencies.
   
    Are you serious? Such a project will never run, because of transitive dependency version conflicts -- precisely that issue we're all trying to use the shade plugin to work around. It's admirable to want your project to "scale', but let's be honest, a single project with more than a fews 10's of modules is already a monolith that should be decomposed.
   
    Can you elaborate on such a use-case? I'm really quite surprised to hear of this as an explicit objective.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven issue #69: MNG-5899 Reactor should use reduced dependency pom

Tunaki
In reply to this post by Tunaki
Github user ifedorenko commented on the issue:

    https://github.com/apache/maven/pull/69
 
    I am dead serious. I am not at liberty to disclose exact numbers, but lets say our main codebase is in the same ballpark if we allow some room for growth. So 5K modules is not "aspirational pipe dream" kind of goal but very much a real-life requirement we have. (if you are interested, I can provide some details about how we use Maven for a project of this size, but maven dev list is probably a better place to talk about it).
   
    Even in opensource [some projects](https://bugs.eclipse.org/bugs/show_bug.cgi?id=515668) apparently have  ~700 modules.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[GitHub] maven issue #69: MNG-5899 Reactor should use reduced dependency pom

Tunaki
In reply to this post by Tunaki
Github user stephenc commented on the issue:

    https://github.com/apache/maven/pull/69
 
    > If the shaded artifact is built as a submodule with a different artifact
    > ID, then the shade plugin can specify its DRP as its main POM. It lets
    > other projects depend on the shaded artifact with only its remaining
    > (unshaded) dependencies.
   
    There are issues with that:
   
    1. There can be issues with a clean build with an empty repository for some reactors (because there is no information to let maven know the shadedArtifactId will be produced by artifactId so in some cases Maven will fail the build early because it "knows" there is no such artifact. Typically this happens in larger reactors where the build sequence can be more "unpredictable" and Maven decides that there is a "short-cut"
   
    2. There can be issues with a build from a non-empty repository, especially when using `-rf`, as you can end up using the `-SNAPSHOT` from a previous execution (which may retain bugs).
   
    My original proposal for a solution was to allow a plugin to advertise dependency reduction of a pom (or additional GAV production).
   
    For dependency reduction, Maven would enforce specific rules such as "no new dependencies" and the build plan would be unmodified.
   
    For additional GAV production, there would have to be strict rules on the new GAV (such as being evaluated based entirely on properties available to general project GAV computation - i.e. nothing injected into the build by another plugin) and similar rules on the new GAV pom (i.e. only ever a subset of the GAV it is being generated from)
   
    That would allow us to create the build plan reflecting the potential behaviours of the shade plugin (as well as the flatten plugin) before any plugin executions but allow plugins to do what is necessary... but my suggestion was shot down last time


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [hidden email] or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Loading...