Uploaded image for project: 'Maven'
  1. Maven
  2. MNG-6207

Create WARNINGs in case of using system scope

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.5.0
    • Fix Version/s: 3.5.2
    • Component/s: None
    • Labels:
      None

      Description

      Currently the documentation has already marked the usage of:

       <dependency>
            <groupId>javax.sql</groupId>
            <artifactId>jdbc-stdext</artifactId>
            <version>2.0</version>
            <scope>system</scope>
            <systemPath>${java.home}/lib/rt.jar</systemPath>
          </dependency>
      

      as deprecated. So we should start to produce WARNING during a build if someone uses it...

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user dejan2609 opened a pull request:

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

          MNG-6207 display build WARNING if deprecated 'system' scope is used

          JIRA ticket: [MNG-6207: Create WARNINGs in case of using system scope](https://issues.apache.org/jira/browse/MNG-6207)

          Note: since *system* scope requires *systemPath* I decided to expand existing and rename systemPath tests; if that kind of change is not acceptable please suggest different approach.

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

          $ git pull https://github.com/dejan2609/maven MNG-6207-system-scope-deprecation-warning

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

          https://github.com/apache/maven/pull/119.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 #119


          commit 0158be5884be228693ecc43f78561e65f0fc59f0
          Author: dejan2609 <dejan2609@gmail.com>
          Date: 2017-05-20T13:42:43Z

          MNG-6207 display build WARNING if deprecated 'system' scope is used


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user dejan2609 opened a pull request: https://github.com/apache/maven/pull/119 MNG-6207 display build WARNING if deprecated 'system' scope is used JIRA ticket: [MNG-6207: Create WARNINGs in case of using system scope] ( https://issues.apache.org/jira/browse/MNG-6207 ) Note: since * system * scope requires * systemPath * I decided to expand existing and rename systemPath tests; if that kind of change is not acceptable please suggest different approach. You can merge this pull request into a Git repository by running: $ git pull https://github.com/dejan2609/maven MNG-6207 -system-scope-deprecation-warning Alternatively you can review and apply these changes as the patch at: https://github.com/apache/maven/pull/119.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 #119 commit 0158be5884be228693ecc43f78561e65f0fc59f0 Author: dejan2609 <dejan2609@gmail.com> Date: 2017-05-20T13:42:43Z MNG-6207 display build WARNING if deprecated 'system' scope is used
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user khmarbaise commented on a diff in the pull request:

          https://github.com/apache/maven/pull/119#discussion_r117611940

          — Diff: maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java —
          @@ -604,17 +609,21 @@ public void testBadImportScopeClassifier()
          "'dependencyManagement.dependencies.dependency.classifier' for test:a:pom:cls must be empty" );
          }

          • public void testSystemPathRefersToProjectBasedir()
            + public void testSystemPathRefersToProjectBasedirAndDeprecatedScope()
              • End diff –

          I think it's better having different test cases. Keeping the old test cases and create new ones for the changed behaviour. That makes it safe not to break previous behaviour,

          Show
          githubbot ASF GitHub Bot added a comment - Github user khmarbaise commented on a diff in the pull request: https://github.com/apache/maven/pull/119#discussion_r117611940 — Diff: maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java — @@ -604,17 +609,21 @@ public void testBadImportScopeClassifier() "'dependencyManagement.dependencies.dependency.classifier' for test:a:pom:cls must be empty" ); } public void testSystemPathRefersToProjectBasedir() + public void testSystemPathRefersToProjectBasedirAndDeprecatedScope() End diff – I think it's better having different test cases. Keeping the old test cases and create new ones for the changed behaviour. That makes it safe not to break previous behaviour,
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user khmarbaise commented on a diff in the pull request:

          https://github.com/apache/maven/pull/119#discussion_r117611970

          — Diff: maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java —
          @@ -411,14 +411,19 @@ public void testIncompleteParent()
          assertTrue( result.getFatals().get( 2 ).contains( "parent.version" ) );
          }

          • public void testHardCodedSystemPath()
            + public void testHardCodedSystemPathAndDeprecatedScope()
              • End diff –

          I think it's better having different test cases. Keeping the old test cases and create new ones for the changed behaviour. That makes it safe not to break previous behaviour,

          Show
          githubbot ASF GitHub Bot added a comment - Github user khmarbaise commented on a diff in the pull request: https://github.com/apache/maven/pull/119#discussion_r117611970 — Diff: maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java — @@ -411,14 +411,19 @@ public void testIncompleteParent() assertTrue( result.getFatals().get( 2 ).contains( "parent.version" ) ); } public void testHardCodedSystemPath() + public void testHardCodedSystemPathAndDeprecatedScope() End diff – I think it's better having different test cases. Keeping the old test cases and create new ones for the changed behaviour. That makes it safe not to break previous behaviour,
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dejan2609 commented on the issue:

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

          New changes are pushed via new commit (all commits will be squashed into one eventually).
          Solution is tested across different Maven versions (*3.0.5, **3.1.1, **3.2.5, **3.3.9* and *3.5.0*).

          Test renaming is recalled; however, it seems kind of reasonable (necessary ?) to fine-grain those test cases instead of adding a new one.
          *Rationale:* since both existing test cases already consume pom.xml files with mutually dependent tags:
          > \<scope>*system*\</scope>
          > \<systemPath>*Some_Path*\</systemPath>

          it seems convenient to carefully reuse those existing test cases, because both tags end up tested anyway (I reckon that adding a new test case with same two tags would be redundant).

          @khmarbaise: let me know what you think (and please forgive me if my rationale is not that rational, I'll be ready to dig deeper and accommodate solution in order to fit standards).

          Bonus question (in the unlikely case that I got it all right): does it make sense to introduce new *ModelBuildingRequest* *VALIDATION_LEVEL_MAVEN* (*3_5 or 3_6*) ?

          Show
          githubbot ASF GitHub Bot added a comment - Github user dejan2609 commented on the issue: https://github.com/apache/maven/pull/119 New changes are pushed via new commit (all commits will be squashed into one eventually). Solution is tested across different Maven versions (* 3.0.5 , **3.1.1 , **3.2.5 , **3.3.9 * and * 3.5.0 *). Test renaming is recalled; however, it seems kind of reasonable (necessary ?) to fine-grain those test cases instead of adding a new one. * Rationale: * since both existing test cases already consume pom.xml files with mutually dependent tags: > \<scope> * system * \</scope> > \<systemPath> * Some_Path * \</systemPath> it seems convenient to carefully reuse those existing test cases, because both tags end up tested anyway (I reckon that adding a new test case with same two tags would be redundant). @khmarbaise: let me know what you think (and please forgive me if my rationale is not that rational, I'll be ready to dig deeper and accommodate solution in order to fit standards). Bonus question (in the unlikely case that I got it all right): does it make sense to introduce new * ModelBuildingRequest * * VALIDATION_LEVEL_MAVEN * ( * 3_5 or 3_6 * ) ?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dejan2609 commented on the issue:

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

          Just to narrow discussion to PR scope only and to eliminate bonus question: there is/was an initiative to increase validation level (*MNG-6075* Increase the model validation level to the next minor level version).

          Show
          githubbot ASF GitHub Bot added a comment - Github user dejan2609 commented on the issue: https://github.com/apache/maven/pull/119 Just to narrow discussion to PR scope only and to eliminate bonus question: there is/was an initiative to increase validation level (* MNG-6075 * Increase the model validation level to the next minor level version).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user khmarbaise commented on the issue:

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

          Could you make a real commit log message like:
          ```
          MNG-6207 Create WARNINGs in case of using system scope
          If needed more description/explanations here.
          ```
          Otherwise I need to do a `git ci --amend ...` which I would like to prevent and keep the original commit...

          Show
          githubbot ASF GitHub Bot added a comment - Github user khmarbaise commented on the issue: https://github.com/apache/maven/pull/119 Could you make a real commit log message like: ``` MNG-6207 Create WARNINGs in case of using system scope If needed more description/explanations here. ``` Otherwise I need to do a `git ci --amend ...` which I would like to prevent and keep the original commit...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dejan2609 commented on the issue:

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

          @khmarbaise done, I altered commit messaged (added note about tests).

          Show
          githubbot ASF GitHub Bot added a comment - Github user dejan2609 commented on the issue: https://github.com/apache/maven/pull/119 @khmarbaise done, I altered commit messaged (added note about tests).
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user khmarbaise commented on the issue:

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

          Sorry...Can please make the PR new based on the current master of the apache-maven, cause I didn't realized that this PR must be done before the other...Thanks in advance...

          Show
          githubbot ASF GitHub Bot added a comment - Github user khmarbaise commented on the issue: https://github.com/apache/maven/pull/119 Sorry...Can please make the PR new based on the current master of the apache-maven, cause I didn't realized that this PR must be done before the other...Thanks in advance...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dejan2609 commented on the issue:

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

          @khmarbaise Ok, I'll open new PR then.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dejan2609 commented on the issue: https://github.com/apache/maven/pull/119 @khmarbaise Ok, I'll open new PR then.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dejan2609 closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user dejan2609 closed the pull request at: https://github.com/apache/maven/pull/119
          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user dejan2609 opened a pull request:

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

          MNG-6207 display deprecation build warning for dependencies with scope 'system' declaration

          Note: this PR is created instead of #119

          @khmarbaise: FYI

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

          $ git pull https://github.com/dejan2609/maven MNG-6207

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

          https://github.com/apache/maven/pull/123.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 #123


          commit 1ddb1f3982a031105384716a1b983228221e00c5
          Author: dejan2609 <dejan2609@gmail.com>
          Date: 2017-05-20T13:42:43Z

          MNG-6207 display deprecation build warning for dependencies with scope 'system' declaration

          Note about tests: existing 'systemPath' related tests are reused/expanded (rationale: scope 'system' and 'systemPath' are mutually dependent)


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user dejan2609 opened a pull request: https://github.com/apache/maven/pull/123 MNG-6207 display deprecation build warning for dependencies with scope 'system' declaration Note: this PR is created instead of #119 @khmarbaise: FYI You can merge this pull request into a Git repository by running: $ git pull https://github.com/dejan2609/maven MNG-6207 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/maven/pull/123.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 #123 commit 1ddb1f3982a031105384716a1b983228221e00c5 Author: dejan2609 <dejan2609@gmail.com> Date: 2017-05-20T13:42:43Z MNG-6207 display deprecation build warning for dependencies with scope 'system' declaration Note about tests: existing 'systemPath' related tests are reused/expanded (rationale: scope 'system' and 'systemPath' are mutually dependent)
          Hide
          khmarbaise Karl Heinz Marbaise added a comment -

          I have get the pull request and had to change the log message to follow our conventions...so now we need to wait for the integration test results..

          Show
          khmarbaise Karl Heinz Marbaise added a comment - I have get the pull request and had to change the log message to follow our conventions...so now we need to wait for the integration test results..
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user khmarbaise commented on the issue:

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

          First build has failed based on [violated checkstyle rules](https://builds.apache.org/view/M-R/view/Maven/job/maven-3.x-jenkinsfile/job/MNG-6207/1/console). So best is before you open a pull request to run a full build via `mvn clean verfiy` on the whole Maven project...the checkstyle rules will be checked as part of the build...

          Show
          githubbot ASF GitHub Bot added a comment - Github user khmarbaise commented on the issue: https://github.com/apache/maven/pull/123 First build has failed based on [violated checkstyle rules] ( https://builds.apache.org/view/M-R/view/Maven/job/maven-3.x-jenkinsfile/job/MNG-6207/1/console ). So best is before you open a pull request to run a full build via `mvn clean verfiy` on the whole Maven project...the checkstyle rules will be checked as part of the build...
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user khmarbaise commented on the issue:

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

          Second build after fixing the checkstyle issues has worked. Integration into master done. You can close the pull request. Thank for your help to make Maven better.

          Show
          githubbot ASF GitHub Bot added a comment - Github user khmarbaise commented on the issue: https://github.com/apache/maven/pull/123 Second build after fixing the checkstyle issues has worked. Integration into master done. You can close the pull request. Thank for your help to make Maven better.
          Show
          khmarbaise Karl Heinz Marbaise added a comment - Done with 12d6471337c7ad067b7762d44050a079829ea26c
          Hide
          hudson Hudson added a comment -

          SUCCESS: Integrated in Jenkins build maven-3.x #1657 (See https://builds.apache.org/job/maven-3.x/1657/)
          MNG-6207 Create WARNINGs in case of using system scope o display (khmarbaise: http://git-wip-us.apache.org/repos/asf/?p=maven.git&a=commit&h=12d6471337c7ad067b7762d44050a079829ea26c)

          • (edit) maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java
          • (edit) maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java
          Show
          hudson Hudson added a comment - SUCCESS: Integrated in Jenkins build maven-3.x #1657 (See https://builds.apache.org/job/maven-3.x/1657/ ) MNG-6207 Create WARNINGs in case of using system scope o display (khmarbaise: http://git-wip-us.apache.org/repos/asf/?p=maven.git&a=commit&h=12d6471337c7ad067b7762d44050a079829ea26c ) (edit) maven-model-builder/src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java (edit) maven-model-builder/src/test/java/org/apache/maven/model/validation/DefaultModelValidatorTest.java
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dejan2609 commented on the issue:

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

          Just for the record: somehow it slipped of my mind that `package` phase doesn't execute integration tests (http://maven.apache.org/ref/3.5.0/maven-core/lifecycles.html#default_Lifecycle) and hence this checkstyle error pass unnoticed on my side:
          > [ERROR] src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java[471:104] (blocks) LeftCurly: '{' should be on a new line.

          Thanx @khmarbaise for this correction and for overall review.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dejan2609 commented on the issue: https://github.com/apache/maven/pull/123 Just for the record: somehow it slipped of my mind that `package` phase doesn't execute integration tests ( http://maven.apache.org/ref/3.5.0/maven-core/lifecycles.html#default_Lifecycle ) and hence this checkstyle error pass unnoticed on my side: > [ERROR] src/main/java/org/apache/maven/model/validation/DefaultModelValidator.java [471:104] (blocks) LeftCurly: '{' should be on a new line. Thanx @khmarbaise for this correction and for overall review.
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dejan2609 closed the pull request at:

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

          Show
          githubbot ASF GitHub Bot added a comment - Github user dejan2609 closed the pull request at: https://github.com/apache/maven/pull/123
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user dejan2609 commented on the issue:

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

          Just on more thing that recalled from my memory: I did tried to execute tests (http://maven.apache.org/core-its/core-it-suite) like this: `mvn clean test -Prun-its` but I received warning that profile doesn't exists:
          > [WARNING] The requested profile "run-its" could not be activated because it does not exist.

          Show
          githubbot ASF GitHub Bot added a comment - Github user dejan2609 commented on the issue: https://github.com/apache/maven/pull/123 Just on more thing that recalled from my memory: I did tried to execute tests ( http://maven.apache.org/core-its/core-it-suite ) like this: `mvn clean test -Prun-its` but I received warning that profile doesn't exists: > [WARNING] The requested profile "run-its" could not be activated because it does not exist.

            People

            • Assignee:
              khmarbaise Karl Heinz Marbaise
              Reporter:
              khmarbaise Karl Heinz Marbaise
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development