Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(general): support Jython 2.7rc2 #488

Merged
merged 1 commit into from
Apr 15, 2015

Conversation

csojinb
Copy link
Contributor

@csojinb csojinb commented Apr 13, 2015

Trying to see if I can get the tests to run with Jython on Travis. Haven't actually implemented Jython support fixes yet. All tests run for me locally, now, except for coverage, which is not compatible with Jython.

Fixes #458

  • make sure Travis runs tests with Jython
  • make Jython tests run with tox looks like there are still problems with support for virtualenv
  • skip coverage tests for Jython -- doesn't cause Travis to fail, though, so maybe okay to just let it be?
  • possibly add to docs somewhere?

@csojinb
Copy link
Contributor Author

csojinb commented Apr 13, 2015

Er, hmm, how do I actually get Travis to run?

@csojinb csojinb changed the title Jython support - TRAVIS CHECK, not ready for review feat(general): support Jython 2.7rc2 Apr 13, 2015
@@ -1,6 +1,7 @@
language: python
sudo: false
install: pip install tox coveralls --use-mirrors
before_install: if [ "$JYTHON" == "true" ]; then travis_scripts/install_jython2.7.sh; fi
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These single-line-ifs all over the place are so ugly. If someone has thoughts on how to do this better, would love to hear them!

I could pull the ifs and all into travis_scripts perhaps.

@csojinb
Copy link
Contributor Author

csojinb commented Apr 13, 2015

Is there a way to optionally skip the coverage tests? It would be nice to be able to skip them for Jython, without causing them to not run by default.

Is there some better solution for coverage's incompatibility with Jython than just skipping those tests?

@csojinb csojinb closed this Apr 13, 2015
@csojinb csojinb reopened this Apr 13, 2015
@csojinb csojinb force-pushed the jython_support branch 2 times, most recently from d8e61a3 to 1caee9d Compare April 13, 2015 22:48
@csojinb
Copy link
Contributor Author

csojinb commented Apr 13, 2015

Argh. I think it didn't actually run with Jython, otherwise I think the coverage tests would have all failed.

@csojinb csojinb force-pushed the jython_support branch 3 times, most recently from e04b330 to 12f23d0 Compare April 14, 2015 15:16
@csojinb
Copy link
Contributor Author

csojinb commented Apr 14, 2015

Okay, here's the deal. I was successful in getting Jython tests to run on Travis here: https://travis-ci.org/falconry/falcon/jobs/58451945. Bad news is that this involved running the Jython tests separately from tox -- so, future changes that are jython-incompatible would break Travis build, but it breaks the uniformity of how version support is tested.

I dug into why tox won't run with Jython 2.7. The issue (obscured through many layers) is that, on startup, tox tries to get the version info for each python interpreter. It spins off a subprocess and basically runs sys.version_info. For some reason, with Jython 2.7, the message gets returned from the subprocess like ">>> ... ... ... >>> {'version_info': (2, 7, 0, 'candidate', 2)}\n>>> ". It returns just a string of the dict with Jython 2.5, so it seems like maybe I should go talk to @jimbaker about it. Spoke with some of the Jython folks, and it looks like it's a problem with Jython not returning properly in non-interactive mode.

Concluding thoughts:
Depending on how badly we want Jython integration to get merged, I can clean up the non-tox solution and capture migration to testing Jython with tox as a separate issue. Or, this can just be put on hold until Jython issues are figured out? @kgriffs what do you think?

@csojinb
Copy link
Contributor Author

csojinb commented Apr 14, 2015

Looks like someone beat me to the bug by a few hours. http://bugs.jython.org/issue2325

@csojinb csojinb force-pushed the jython_support branch 2 times, most recently from 1e78057 to 336059e Compare April 14, 2015 20:08
Does not appear to require any changes to the actual library code, only
to the tests

Jython 2.7 does not have virtualenv support yet, so it can't play with tox.
Once that support comes through, .travis.yml should be able to be returned
to its former elegance

- Ignore Jython py.class files
- Run tests with Jython on Travis
- Use threading instead of multiprocessing, because Jython does not have
a multiprocessing module
- Fix utf encoding specification
  - Apparently Jython is more finicky about the formatting of this
- Accommodate Java time precision in middleware tests
  - Java does not do timestamps with better than ms precision

Closes falconry#458
@csojinb
Copy link
Contributor Author

csojinb commented Apr 14, 2015

@kgriffs I think this is ready to go, pending review

@kgriffs
Copy link
Member

kgriffs commented Apr 15, 2015

LGTM

@jmvrbanac
Copy link
Member

👍

jmvrbanac added a commit that referenced this pull request Apr 15, 2015
feat(general): support Jython 2.7rc2
@jmvrbanac jmvrbanac merged commit 67c22a6 into falconry:master Apr 15, 2015
@csojinb csojinb deleted the jython_support branch June 2, 2016 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Jython
4 participants