You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by jodastephen <gi...@git.apache.org> on 2017/10/10 17:10:43 UTC
[GitHub] commons-lang pull request #299: Add module-info for Java 9
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 <st...@opengamma.com>
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
----
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by jodastephen <gi...@git.apache.org>.
Github user jodastephen commented on the issue:
https://github.com/apache/commons-lang/pull/299
Its not just compiling, there are issues at runtime still AFAIK, especially with Java EE and older bytecode libraries.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by jodastephen <gi...@git.apache.org>.
Github user jodastephen commented on the issue:
https://github.com/apache/commons-lang/pull/299
Adding `module-info` causes some older/Android projects to fail as they can't handle the class format. So, projects are left with the choice of moving forwards or breaking these older/Android projects.
This issue is also tied up in the need to modernise the commons parent pom.xml for Java 9 and later.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by sigursoft <gi...@git.apache.org>.
Github user sigursoft commented on the issue:
https://github.com/apache/commons-lang/pull/299
Hi guys,
What's the status on this pull request? 😄
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by jodastephen <gi...@git.apache.org>.
Github user jodastephen commented on the issue:
https://github.com/apache/commons-lang/pull/299
@namannigam You need to check it out and run `mvn clean install site` on Java 9.
---
[GitHub] commons-lang pull request #299: Add module-info for Java 9
Posted by stokito <gi...@git.apache.org>.
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 issue #299: Add module-info for Java 9
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/299
[](https://coveralls.io/builds/13771246)
Coverage increased (+0.01%) to 95.199% when pulling **07c0c408e72836231ef262f565761778e3e9e103 on jodastephen:module-info** into **1f0dfc31b51a445eb2cfbee5321800cf51e10b67 on apache:master**.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/299
[](https://coveralls.io/builds/13758874)
Coverage increased (+0.01%) to 95.199% when pulling **cb5a17949bf941118a80ef05ccd5a77717779792 on jodastephen:module-info** into **1f0dfc31b51a445eb2cfbee5321800cf51e10b67 on apache:master**.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by namannigam <gi...@git.apache.org>.
Github user namannigam commented on the issue:
https://github.com/apache/commons-lang/pull/299
@jodastephen What's the error that you get with site? Any links to the build?
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by jodastephen <gi...@git.apache.org>.
Github user jodastephen commented on the issue:
https://github.com/apache/commons-lang/pull/299
There is a lot of complexity in commons-parent and commons generally. Getting a build to work for Joda was hard enough, and there I am willing to insist on releases being done on Java 9 or later. There needs to be a lot more work on the commons build before `module-info` should be considered, however at the moment its not clear as to whether doing that work would be positive (due to Android, Jakarta EE issues etc)
---
[GitHub] commons-lang pull request #299: Add module-info for Java 9
Posted by namannigam <gi...@git.apache.org>.
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 issue #299: Add module-info for Java 9
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/299
[](https://coveralls.io/builds/13963478)
Coverage decreased (-0.02%) to 95.163% when pulling **313723da37a87a683683ff5edab139cf10612573 on jodastephen:module-info** into **1f0dfc31b51a445eb2cfbee5321800cf51e10b67 on apache:master**.
---
[GitHub] commons-lang pull request #299: Add module-info for Java 9
Posted by PascalSchumacher <gi...@git.apache.org>.
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
Posted by britter <gi...@git.apache.org>.
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
Posted by jodastephen <gi...@git.apache.org>.
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
Posted by jodastephen <gi...@git.apache.org>.
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 issue #299: Add module-info for Java 9
Posted by johnou <gi...@git.apache.org>.
Github user johnou commented on the issue:
https://github.com/apache/commons-lang/pull/299
@jodastephen can you be more specific?
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/299
[](https://coveralls.io/builds/13919900)
Coverage decreased (-0.02%) to 95.163% when pulling **8362e24c2edd5a7cb88fd9b74c71475462e2fb64 on jodastephen:module-info** into **1f0dfc31b51a445eb2cfbee5321800cf51e10b67 on apache:master**.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by britter <gi...@git.apache.org>.
Github user britter commented on the issue:
https://github.com/apache/commons-lang/pull/299
I'll bring this up a last time on the ML to make sure nobody has objections against merging this.
---
[GitHub] commons-lang pull request #299: Add module-info for Java 9
Posted by stokito <gi...@git.apache.org>.
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 issue #299: Add module-info for Java 9
Posted by jodastephen <gi...@git.apache.org>.
Github user jodastephen commented on the issue:
https://github.com/apache/commons-lang/pull/299
`mvn install` works fine on each JDK version AFAICT. `mvn site` still breaks on cobertura on Java 9 and I'm not sure I can work around it without removing it from the parent pom.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by coveralls <gi...@git.apache.org>.
Github user coveralls commented on the issue:
https://github.com/apache/commons-lang/pull/299
[](https://coveralls.io/builds/13771842)
Coverage increased (+0.01%) to 95.199% when pulling **886b26aa09efc1a4bfca3470e33b952399f18f6d on jodastephen:module-info** into **1f0dfc31b51a445eb2cfbee5321800cf51e10b67 on apache:master**.
---
[GitHub] commons-lang pull request #299: Add module-info for Java 9
Posted by jodastephen <gi...@git.apache.org>.
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 issue #299: Add module-info for Java 9
Posted by michaelsavich <gi...@git.apache.org>.
Github user michaelsavich commented on the issue:
https://github.com/apache/commons-lang/pull/299
Is there any way to rewrite AbstractCircuitBreaker to eliminate the dependency on java.desktop? Java.desktop is quite a large dependency to have for the sake of one class.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on the issue:
https://github.com/apache/commons-lang/pull/299
@michaelsavich I have a proposal in a branch, submitted some months ago https://github.com/apache/commons-lang/pull/275
Waiting for feedback to prepare deprecated annotations in place, and merge in a major release (I believe it breaks BC)
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by britter <gi...@git.apache.org>.
Github user britter commented on the issue:
https://github.com/apache/commons-lang/pull/299
Awesome! Would be create if you could create and reference a JIRA ticket as described in CONTRIBUTING.md, so this will show up in our release notes.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by jodastephen <gi...@git.apache.org>.
Github user jodastephen commented on the issue:
https://github.com/apache/commons-lang/pull/299
@michaelsavich , this PR does not create a full dependency on `java.desktop`. The dependency is expressed as `requires static`, which means that it will not be pulled in by default. Users that want to use the two broken classes on Java 9 in module mode (a very small percentage of commons-lang users) will need to manually include the `java.desktop` dependency. As such, there is no urgency to get `AbstractCircuitBreaker` removed.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by johnou <gi...@git.apache.org>.
Github user johnou commented on the issue:
https://github.com/apache/commons-lang/pull/299
@jodastephen have you checked out https://github.com/moditect/moditect ?
Moditect uses Objweb-ASM to compile the module-info.class which means you can support Java 1.7 / 1.8 for Android.
---
[GitHub] commons-lang issue #299: Add module-info for Java 9
Posted by michaelsavich <gi...@git.apache.org>.
Github user michaelsavich commented on the issue:
https://github.com/apache/commons-lang/pull/299
Ah, gotcha. That makes sense.
---
[GitHub] commons-lang pull request #299: Add module-info for Java 9
Posted by lploom <gi...@git.apache.org>.
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.
---