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

Add Rails 4 compatibility under JRuby #102

Merged
merged 1 commit into from
Jul 29, 2014
Merged

Conversation

GUI
Copy link
Contributor

@GUI GUI commented Mar 11, 2014

This adds Rails 4 compatibility under JRuby and the activerecord-jdbc-adapter. This addresses #76. Fixing this mostly involved making the adapter skip the OID usage stuff, since the jdbc adapter doesn't utilize that.

This also fixes an issue I discovered while running the test suite: Rails 3.1 compatibility was broken under JRuby if you were using the activerecord-jdbc-adapter 1.3.x (but it still worked fine under 1.2.x). I'm not sure how many people are actually using this likely obscure combination of versions, but the fix was relatively straightforward, so I fixed that too.

Note that the Rails 4 tests will currently fail on Travis due to this rgeo-activerecord issue: rgeo/rgeo-activerecord#12 But that issues affects all ruby platforms under Rails 4, not just JRuby. So I don't think you'll get clean tests until that issue is addressed in rgeo-activerecord, but in my own local testing, I get clean tests when building against that rgeo-activerecord branch.

@teeparham
Copy link
Member

Thank you for this PR. To get Rails 4 working, I temporarily dropped JRuby support in master. At this point, your branch will be difficult to merge into master. Would you be willing to take a shot at adding back JRuby support to the master branch? I hate to say start over, but there are missing files, lots of whitespace changes, etc., so I suspect that a clean branch + some copypasta will be the best course.

Rails 3.x is not supported in master, so the 3.x fix doesn't need to be included there. I will cherry-pick your Rails 3.x commit onto the 0.6-stable branch.

@teeparham
Copy link
Member

I cherry-picked the 3.1 fix onto the 0.6-stable branch: a3f7e8604425404f

@GUI
Copy link
Contributor Author

GUI commented May 6, 2014

Yup, I can definitely try to look at re-adding JRuby support to master. Getting everything working while supporting all the different versions of Rails was getting a little unwieldy, so I agree taking a fresh look will probably be easiest. If we're just targeting Rails 4 on master, I think JRuby support should be fairly straightforward to add. I'm not exactly sure when I'll be able to get to this, but hopefully sometime in the next week or two.

Would you also be interested in a clean JRuby patch for the 0.7.x branch? It looks like that branch is for Rails 4.0 while master/1.0.0 is for Rails 4.1. Most of our JRuby projects that need postgis support are currently still using 4.0 and we haven't scheduled upgrades to 4.1 yet. So at least in the near term, I have in interest in getting JRuby support on both 0.7 and master. I can work on separate, clean PRs for each branch, but would you be interested in both? Or are you only interested in master at this point?

But I'm excited to see things pick up again for this project. Thanks for becoming the new maintainer and a big thanks to @dazuma for everything he's done!

@teeparham
Copy link
Member

Great. Yes, I would like to get patches for both Rails 4.0 & for Rails 4.1. If you want to work off the 0.7-stable branch for the 4.0.x compatibility, that would probably be easiest (and most useful for you, it appears). So just make sure the target branch of the pull request is 0.7-stable. Then it should be easy to cherry-pick the commits onto master, as I suspect nothing will be different.

Thanks for your help!

As an aside, I'd prefer not to have different branches for 4.0 & 4.1, but that's what happened. These 2 commits worked with 4.1 but not 4.0 & I just plowed ahead:

49c1e5840da
f47aeaf1fab7

@teeparham teeparham mentioned this pull request May 7, 2014
@teeparham
Copy link
Member

@GUI This commit in master (add74ada5ffac) allows us to support 4.0 and 4.1 in master. So you can work off the master branch now. I'll be releasing this as 1.1.0 soon.

@trinode
Copy link

trinode commented Jun 30, 2014

How's the progress on this? I've just found out this project existed. I'm trying to work out if I should use this or raw SQL for queries for spacial things in JRuby on Rails 4.1. (I have no choice on platform).

@clarketus
Copy link

Hey guys, I'm also blocked on upgrading to rails 4 due to the same reasons as the previous comment!

Whats involved in getting this merged? Do you need any help? Cheers.

@GUI
Copy link
Contributor Author

GUI commented Jul 28, 2014

Sorry for the long delay in getting to this. This pull request should now be updated to bring JRuby compatibility with the current version of this gem.

@teeparham Could you give this another look? Adding JRuby compatibility was actually way more straightforward now that this gem is only supporting Rails 4. There have also been some updates to the jdbc adapter in the meantime that also make this more straightforward.

A few extra notes:

  • This merges in Use Appraisal to make local testing with multiple Rails versions easier #103, the usage of the Appraisal gem. This is basically just a formalized way to generate gemfiles and run multiple tests when dealing with multiple versions of Rails. The reason I merged this in was that the rake compatibility task didn't work under JRuby for some strange reason (it didn't seem to like the shelling out the rake task was doing). The only real difference in usage with this is that you run rake appraisal instead. So if you're okay with this, and this PR gets merged in, you can close Use Appraisal to make local testing with multiple Rails versions easier #103.
  • Travis is currently having issues with Rubinius and installing gems: Rubinius - corrupt cached gems travis-ci/travis-ci#2542 It looked like the current .travis.yml file was supposed to allow for rubinius failures, but the travis config wasn't quite right, so the build was failing. I fixed the travis config to allow rubinius failures. This makes the build pass, but obviously if allowing rubinius failures wasn't intentional, then you might want to remove that whole block from the .travis.yml.
  • I think the overall diff on this PR should actually be pretty straightforward, but the git history might be a bit of a mess (since the jruby stuff got redone and then I also pulled in the appraisal stuff). If you'd prefer I rebase this PR into a single commit, just let me know.

@teeparham
Copy link
Member

@GUI Thanks so much for doing this. I reviewed and it looks good to me.

I agree: the overall diff of this PR is straightforward but the git history is not so readable. Here's a proposal that may be the easiest way to clean up the commit history:

  • Add the JRuby changes as a new PR branched off the latest master.
  • Add the Appraisal changes as a new PR branched off the latest master.

You could copy over your versions of the changed files from your jruby-rails4 branch to the new PR branches to get this done quickly. Separate commits per file are ok, or group them together as best makes sense to you.

If that sounds reasonable, go for it & I'll merge asap. You could also squash/re-order commits via an interactive rebase, but I suspect it would be more work than simple copy/paste.

Thanks again for you help!

@GUI
Copy link
Contributor Author

GUI commented Jul 28, 2014

@teeparham Thanks for giving this a gander! I can definitely get this cleaned up into a more reasonable git commit history. I'll try to get that sorted later today or tomorrow at the latest.

@GUI
Copy link
Contributor Author

GUI commented Jul 29, 2014

@teeparham Okay, I think this is all squashed and split up now into 3 separate PRs with single commits:

#102 and #103 aren't passing Travis, due to the aforementioned Rubinius issue, which I made a distinct PR. But once they all come together, I think Travis should be happy.

I tried to structure PRs to minimize any merge conflicts if you want to take them all, but one thing to note is that after merging the JRuby and Appraisal stuff together, you'll need to generate new gemfiles (since the appraisal branch's gemfiles don't have the jruby gems included). Appraisal should make this easy, so once both are merged in, you can do this with bundle exec appraisal install. Give me a shout if you have any additional questions.

Thanks!

@teeparham teeparham merged commit 0c00ffb into rgeo:master Jul 29, 2014
@teeparham
Copy link
Member

Thanks @GUI!

Rails 4 JRuby folks: please try the latest master. I'll release version 2.2.0 soon.

🎆

@GUI
Copy link
Contributor Author

GUI commented Jul 29, 2014

Awesome, thanks so much for the quick merge! I'll try to get a couple of our JRuby apps updated to this final merged version that's on master this week to give it some additional testing.

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.

4 participants