search

twitter-archive / commons

Twitter common libraries for python and the JVM (deprecated)
http://twitter.github.com/commons
Other
2.1k stars 565 forks source link

com.twitter.common#jar-tool 0.1.8 & 0.1.9 are java 1.7 classfile jars #361

Closed jsirois closed 9 years ago

jsirois commented 9 years ago

This breaks twitter/commons itself which tries to CI against 1.6 and 1.7 and blocks upgrading the twitter/commons jar-tool from 0.1.7. Either an 0.1.10 needs to be published using -source 6 -target 6 or else the twitter/commons travis-ci needs to be restricted to 1.7.

jsirois commented 9 years ago

CI example here: https://travis-ci.org/twitter/commons/jobs/54192995 Important bit of the log:

[== Running python tests ==]
...
03:17:33 00:01   [test]
03:17:33 00:01     [run_prep_command]
03:17:33 00:01     [test]
03:17:33 00:01     [pytest]
03:17:35 00:03       [run]
...
                     ============== test session starts ===============
                     platform linux2 -- Python 2.7.3 -- py-1.4.26 -- pytest-2.6.4
                     plugins: cov, timeout
                     collected 1 items 

                     tests/python/twitter/common/zookeeper/kazoo_client_test.py F

                     ==================== FAILURES ====================
                     __________________ test_metrics __________________

                         def test_metrics():
                     >     with ZookeeperServer() as server:
                             event = threading.Event()

                     tests/python/twitter/common/zookeeper/kazoo_client_test.py:28: 
                     _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
                     /tmp/tmp27vSyP/twitter/common/zookeeper/test_server.py:101: in __init__
                         self.angrybird = self.setup_thrift()
                     _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

                     self = <twitter.common.zookeeper.test_server.ZookeeperServer object at 0x365cb90>

                         def setup_thrift(self):
                           if self._service is None:
                             time.sleep(self.INITIAL_BACKOFF)
                             for _ in range(self.CONNECT_RETRIES):
                               try:
                                 self._service = make_client(ZooKeeperThriftServer, 'localhost', self.thrift_port,
                                   protocol=TFinagleProtocol)
                                 break
                               except TTransportException:
                                 time.sleep(self.CONNECT_BACKOFF_SECS)
                                 continue
                             else:
                     >         raise self.NotStarted('Could not start Zookeeper cluster!')
                     E         NotStarted: Could not start Zookeeper cluster!

                     /tmp/tmp27vSyP/twitter/common/zookeeper/test_server.py:130: NotStarted
                     -------------- Captured stdout call --------------

                     03:18:12 00:00 [main]
                                    (To run a reporting server: ./pants server)
...
                     03:18:37 00:25   [binary]
                     03:18:37 00:25     [python-binary-create]
                     03:18:37 00:25     [binary]
                                        creating dist/angrybird.jar
                     03:18:37 00:25       [create-monolithic-jar]
                     03:18:37 00:25         [add-internal-classes]
                     03:18:37 00:25         [add-dependency-jars]
                     03:18:37 00:25         [bootstrap-jar-tool]
                     03:19:01 00:49         [jar-tool]
                                            ==== stderr ====
                                            java.lang.UnsupportedClassVersionError: com/twitter/common/jar/tool/Main : Unsupported major.minor version 51.0
                                                at java.lang.ClassLoader.defineClass1(Native Method)
                                                at java.lang.ClassLoader.defineClass(ClassLoader.java:643)
                                                at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
                                                at java.net.URLClassLoader.defineClass(URLClassLoader.java:277)
                                                at java.net.URLClassLoader.access$000(URLClassLoader.java:73)
                                                at java.net.URLClassLoader$1.run(URLClassLoader.java:212)
                                                at java.security.AccessController.doPrivileged(Native Method)
                                                at java.net.URLClassLoader.findClass(URLClassLoader.java:205)
                                                at java.lang.ClassLoader.loadClass(ClassLoader.java:323)
                                                at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:294)
                                                at java.lang.ClassLoader.loadClass(ClassLoader.java:268)
                                                at java.lang.Class.forName0(Native Method)
                                                at java.lang.Class.forName(Class.java:191)
                                                at com.martiansoftware.nailgun.NGSession.run(NGSession.java:242)
...
jinfeng commented 9 years ago

May I ask what would be the consequences if we restrict CI to 1.7? Do we know any client of these artifacts require 1.6?

jsirois commented 9 years ago

There may be none. The customers are the transitive jvm customers of pants as a build tool so it would be best to query panst-devel. I think Square uses all of 6, 7 & 8 in their codebases, but I'm not sure if they use 1.6 as a runtime - they may use 1.7 & 1.8 or just 1.8 for the runtime. But @ericzundel will know for sure. In general I'd like to maintain compatibility with 1.6 in pants since we've always supported it and this tool just uses 1.6 language features. At some point pants really needs to fork its commons tools and own them over in pantsbuild so if Twitter needs commons publications to be 1.7 this might be a good time to fork jar-tool, compiler & specs runner.

ericzundel commented 9 years ago

Square's internal repos are using mostly 1.8 with some 1.7 usage. We're now rid of our 1.6 dependencies (in our main java repo anyway). A agree its good to keep a low level build tool like this at Java 1.6 unless we have a compelling reason to use more recent language features.

jsirois commented 9 years ago

Erics response made me think more and I think its actually not viable for Twitter to start publishing the commons java 1.6 codebase (zookeeper, args, application, etc) in 1.7. That should break folks. I know AirBnB, Compass and Aurora all use bits of the commons code. There is no telling if they or their customers are forced to stay on 1.6 runtimes.

jinfeng commented 9 years ago

0.1.10 with java6 compatible bits is now published at:

http://maven.twttr.com/com/twitter/common/jar-tool/0.1.10/

jsirois commented 9 years ago

Thanks @jinfeng!

jsirois commented 9 years ago

OK - the transitive closure (or some subset) of the jar-tool's deps actually needed to be re-published:

22:28:54 00:33         [jar-tool]
                                            ==== stderr ====
                                            java.lang.UnsupportedClassVersionError: com/twitter/common/args/Arg : Unsupported major.minor version 51.0
                                                at java.lang.ClassLoader.defineClass1(Native Method)
                                                at java.lang.ClassLoader.defineClass(ClassLoader.java:643)
                                                at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
                                                at java.net.URLClassLoader.defineClass(URLClassLoader.java:277)
                                                at java.net.URLClassLoader.access$000(URLClassLoader.java:73)
                                                at java.net.URLClassLoader$1.run(URLClassLoader.java:212)
                                                at java.security.AccessController.doPrivileged(Native Method)
                                                at java.net.URLClassLoader.findClass(URLClassLoader.java:205)
                                                at java.lang.ClassLoader.loadClass(ClassLoader.java:323)
                                                at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:294)
                                                at java.lang.ClassLoader.loadClass(ClassLoader.java:268)
                                                at com.twitter.common.logging.RootLogConfig.<clinit>(RootLogConfig.java:49)
                                                at com.twitter.common.jar.tool.Main.main(Main.java:388)
                                                at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
                                                at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
                                                at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
                                                at java.lang.reflect.Method.invoke(Method.java:622)
                                                at com.martiansoftware.nailgun.NGSession.run(NGSession.java:280)

So jar-tool is now good, but it still depends on an args that java-7 only.

Before going further I'll ask on pants-devel about Twitter, twitter/commons and java 6 compat for all tools. If it turns out Twitter is now routinely only publishing to a java 7 minimum we'll continually get breaks like this and the time for pantsbuild to fork / move this code into its own repo is now.

jsirois commented 9 years ago

Upgraded to org.pantsbuild maven artifacts in https://github.com/twitter/commons/commit/da94c931655a66d9ed9599f7a2ce4a03c9da2d4f