[GitHub] commons-lang pull request #299: Add module-info for Java 9

[lploom]

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

https://github.com/apache/commons-lang/pull/299#discussion_r221431119
  
--- Diff: src/main/java/module-info.java ---
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Apache Commons Lang provides utility classes for the core of Java.
+ */
+module org.apache.commons.lang3 {
+
+// see AbstractCircuitBreaker
+requires static java.desktop;
--- End diff --

Isn't the best option to refactor the concurrent package to separate 
project? Doesn't seem like a great fit here anyhow.


---

[GitHub] commons-lang pull request #299: Add module-info for Java 9

[jodastephen]

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

https://github.com/apache/commons-lang/pull/299#discussion_r147859782
  
--- Diff: .travis.yml ---
@@ -21,8 +21,19 @@ jdk:
   - oraclejdk8
   - oraclejdk9
 
+cache:
+  directories:
+- "$HOME/.m2/repository"
+
+before_cache:
+  - rm -rf $HOME/.m2/repository/org/apache/commons/commons-lang3
+
+install:
+  - mvn --version
+
 script:
-  - mvn
+  - mvn -e -B
 
 after_success:
-  - mvn clean cobertura:cobertura coveralls:report -Ptravis-cobertura
+  - if [ $TRAVIS_JDK_VERSION == "openjdk7" ] || [ $TRAVIS_JDK_VERSION == 
"oraclejdk8" ]; then mvn -e -B clean cobertura:cobertura coveralls:report 
-Ptravis-cobertura; fi
+  - if [ $TRAVIS_JDK_VERSION == "oraclejdk9" ]; then mvn -e -B clean 
cobertura:cobertura -Ptravis-cobertura; fi
--- End diff --

The right answer will be to move from Cobertura to JaCoCo (as JaCoCo is the 
more alive project). But right now, Coveralls also doesn't work with JaCoCo.


---

[GitHub] commons-lang pull request #299: Add module-info for Java 9

[PascalSchumacher]

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

https://github.com/apache/commons-lang/pull/299#discussion_r147560886
  
--- Diff: .travis.yml ---
@@ -21,8 +21,19 @@ jdk:
   - oraclejdk8
   - oraclejdk9
 
+cache:
+  directories:
+- "$HOME/.m2/repository"
+
+before_cache:
+  - rm -rf $HOME/.m2/repository/org/apache/commons/commons-lang3
+
+install:
+  - mvn --version
+
 script:
-  - mvn
+  - mvn -e -B
 
 after_success:
-  - mvn clean cobertura:cobertura coveralls:report -Ptravis-cobertura
+  - if [ $TRAVIS_JDK_VERSION == "openjdk7" ] || [ $TRAVIS_JDK_VERSION == 
"oraclejdk8" ]; then mvn -e -B clean cobertura:cobertura coveralls:report 
-Ptravis-cobertura; fi
+  - if [ $TRAVIS_JDK_VERSION == "oraclejdk9" ]; then mvn -e -B clean 
cobertura:cobertura -Ptravis-cobertura; fi
--- End diff --

Nitpick: I think we can remove this line, because running `cobertura` 
without uploading to `coveralls` mostly just wastes cpu cycles.


---

[GitHub] commons-lang pull request #299: Add module-info for Java 9

[jodastephen]

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

https://github.com/apache/commons-lang/pull/299#discussion_r147424665
  
--- Diff: src/main/java/module-info.java ---
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Apache Commons Lang provides utility classes for the core of Java.
+ */
+module org.apache.commons.lang3 {
+
+// see AbstractCircuitBreaker
+requires static java.desktop;
--- End diff --

Because this is a massive dependency, and we need to avoid dragging it all 
in for just a small percentage of users. Using `requires static` is an abuse of 
the module system, but the best option available. `requires transitive` would 
be the right solution normally.


---

[GitHub] commons-lang pull request #299: Add module-info for Java 9

[namannigam]

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

https://github.com/apache/commons-lang/pull/299#discussion_r147422522
  
--- Diff: src/main/java/module-info.java ---
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Apache Commons Lang provides utility classes for the core of Java.
+ */
+module org.apache.commons.lang3 {
+
+// see AbstractCircuitBreaker
+requires static java.desktop;
--- End diff --

a small difference and I am just curious to know, why is this not 
`transitive` which was what I saw using `jdeps --generate-module-info`?


---

[GitHub] commons-lang pull request #299: Add module-info for Java 9

[britter]

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

https://github.com/apache/commons-lang/pull/299#discussion_r144693421
  
--- Diff: .travis.yml ---
@@ -17,12 +17,10 @@ language: java
 sudo: false
 
 jdk:
-  - openjdk7
--- End diff --

I think we should discuss this on the developer mailing list.


---

[GitHub] commons-lang pull request #299: Add module-info for Java 9

[stokito]

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

https://github.com/apache/commons-lang/pull/299#discussion_r144134867
  
--- Diff: .travis.yml ---
@@ -17,12 +17,10 @@ language: java
 sudo: false
 
 jdk:
-  - openjdk7
--- End diff --

Sounds sad :( Especially because, I guess, jdk8 compiler willn't just 
ignore the `module-info.java`.
Maybe some other projects will even try to generate the `module-info.java`


---

[GitHub] commons-lang pull request #299: Add module-info for Java 9

[jodastephen]

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

https://github.com/apache/commons-lang/pull/299#discussion_r144119796
  
--- Diff: .travis.yml ---
@@ -17,12 +17,10 @@ language: java
 sudo: false
 
 jdk:
-  - openjdk7
--- End diff --

With the proposed change, the build can only occur on Java 9. (Java 9 is 
needed to compile `module-info.java`).


---

[GitHub] commons-lang pull request #299: Add module-info for Java 9

[stokito]

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

https://github.com/apache/commons-lang/pull/299#discussion_r143869192
  
--- Diff: .travis.yml ---
@@ -17,12 +17,10 @@ language: java
 sudo: false
 
 jdk:
-  - openjdk7
--- End diff --

Why did you removed the build for jdk7 and 8?
If lang3 still support them then it should be runned tests on 7 and 8


---

[GitHub] commons-lang pull request #299: Add module-info for Java 9

[jodastephen]

GitHub user jodastephen opened a pull request:

https://github.com/apache/commons-lang/pull/299

Add module-info for Java 9

Add the module-info.java file
Make the appropriate changes to pom.xml

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

$ git pull https://github.com/jodastephen/commons-lang module-info

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

https://github.com/apache/commons-lang/pull/299.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 #299


commit 8f5aeed9db2ec0fed0e81ebd24a9608d0d4d68d6
Author: Stephen Colebourne 
Date:   2017-10-10T16:56:41Z

Add module-info for Java 9

Add the module-info.java file
Make the appropriate changes to pom.xml




---