JRuby on Windows #480

AndyObtiva · 2020-08-06T04:58:47Z

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

Ensure all commits include DCO sign-off.
Ensure that your contributions pass unit testing.
Ensure that your contributions contain documentation if applicable.

Description

Please describe your pull request.

Hi, I encountered an issue with using git gem (via jeweler gem) on Windows running in JRuby.

Git::GitExecuteError: git '-c' 'color.ui=false' version   2>&1:git: ''-c'' is not a git command. See 'git --help'.
C:/Users/User/code/ruby-git/lib/git/lib.rb:989:in `command'
C:/Users/User/code/ruby-git/lib/git/lib.rb:895:in `current_command_version'
C:/Users/User/code/ruby-git/lib/git/lib.rb:905:in `meets_required_version?'
C:/Users/User/code/ruby-git/lib/git.rb:26:in `<main>'

It turns out it was not escaping strings properly because it was running in JRuby instead of CRuby, thus not detecting Windows correctly via RUBY_PLATFORM. I fixed it by adding a check for RUBY_DESCRIPTION too.

Cheers.

jcouball

In addition to the code comment, is there a test that fails without this change? The JRuby build seems to have been passing. If there isn't, can you please make one?

jcouball · 2020-08-16T21:04:37Z

lib/git/lib.rb

+      # Check if on Windows via RUBY_PLATFORM (CRuby) and RUBY_DESCRIPTION (JRuby)
+      win_platform_regex = /mingw|mswin/
+      return "'#{s && s.to_s.gsub('\'','\'"\'"\'')}'" if RUBY_PLATFORM !~ win_platform_regex && RUBY_DESCRIPTION !~ win_platform_regex
+


How about shortening this to the more intention revealing:

def escape(s) return escape_for_windows(s) if windows_platform? escape_for_sh(s) # not sure what the best name for this method is: are we escaping for sh? linux? linux_shell? end

Then move the existing code to fill in the new methods referenced above. Then add a test (or more) for each method.

Sure. I agree that the suggested refactorings into intention revealing methods make the code a lot more readable/understandable.

jcouball · 2020-08-16T22:21:49Z

I re-ran the build and the status was updated. Not sure why it wasn't updating before.

AndyObtiva · 2020-08-17T17:19:14Z

In addition to the code comment, is there a test that fails without this change? The JRuby build seems to have been passing. If there isn't, can you please make one?

Good question. As you probably noticed, I've been trying to update the Travis CI build to include Windows. I think the current tests are sufficient if run on Windows as they test that escape(s) method indirectly via public methods instead. The reason the jruby build was passing is because it was running on Linux. It's a Windows compatibility problem after all on top of jruby.

I was able to add a Windows Ruby build and got 5 failures and a number of errors in it. I'm hunting them one by one on my machine right now.

The Windows JRuby build seems like no dice at the moment as Travis CI doesn't officially support it, so I'm manually scripting installation of JRuby with a Travis CI shell script, but if I don't get it working, the Windows Ruby build would probably suffice and should help quite a bit in testing Windows support in general with the only difference in JRuby being to check the RUBY_DESCRIPTION on top of RUBY_PLATFORM to ascertain Windows from non-Windows environments.

This is a fun break from some tough work I was busy with last week, so I'm more than happy to involve myself with this. I hope you're not in a rush. I'm taking my time given that I need a break from last week.

Thanks for acknowledging the bug and for providing code refactoring feedback that I agree makes the code more readable. I look forward to addressing all failing Windows Ruby tests and performing the refactoring in the foreseeable future.

Godspeed.

p.s. Once done, I'll make sure to rebase the latest code from trunk/master too since it's been a little while since I reported this..

jcouball · 2020-08-17T18:34:43Z

I hope you're not in a rush.

No rush, carry on at your own pace.

jcouball · 2020-08-17T18:36:52Z

If you want to break down this PR, you could add the new Travis CI builds first, in a separate PR. You can marking the Windows Ruby build(s) as optional so they don't impact others. It would be nice to have Ruby 2.7 added sooner than later.

AndyObtiva · 2020-08-19T14:51:31Z

If you want to break down this PR, you could add the new Travis CI builds first, in a separate PR. You can marking the Windows Ruby build(s) as optional so they don't impact others. It would be nice to have Ruby 2.7 added sooner than later.

Done.

I marked the Windows Ruby build(s) as optional.

I got both Ruby Windows and JRuby Windows working.

I took off Ruby 2.7 (it's a in a separate pull request now).

In summary, the changes mostly focus on removing reliance on bash shell commands that are incompatible with Windows, such as du (replaced with a pure Ruby repo size calculation), pwd (replaced with Ruby's higher-level abstract FileUtils.pwd), tar (replaced with pure Ruby gem "minitar", only needed for tests), and gzip (replaced with built-in Ruby Zlib implementation). The changes also fixed a few discovered minor test issues, including ones relating to Windows paths starting with directory letter (e.g. "C:"). Last but not least, I did the refactoring you recommended regarding escape(s).

Thanks for making the Ruby git library available to the open-source community.

Godspeed.

jcouball

I just had one comment about the encoding tests.

Also, can you please catch up to master and squash commits when you are ready?

jcouball · 2020-09-06T00:17:17Z

tests/units/test_diff_non_default_encoding.rb

@@ -35,13 +35,15 @@ def setup
  def test_diff_with_greek_encoding
    d = @git.diff
    patch = d.patch
+    return unless Encoding.default_external == (Encoding::UTF_8 rescue Encoding::UTF8) # skip test on Windows / check UTF8 in JRuby instead


I don't think this is quite right. What do you think about this:

skip 'Encoding tests do not work in Windows' if windows_platform?

On other platforms, these tests should work not matter what the external encoding is.

I forgot to mention... I have not ever seen rescue used in this way. Can you point me to something that explains how you are using it?

About your first question, if you can figure out a better way to skip tests on Windows, then power to you! After all, they do not work in their current form inside the Windows Command Prompt or Git Bash and I did not feel it was important enough for me to invest more time into it (especially given that there was zero coverage before to begin with, so any coverage now is better than nothing)

About your second question, I learned this from a senior Ruby developer called Chris Sepic more than a decade ago while working at this Chicago suburb company, Leapfrog Online.

Basically, in rare cases where you do not need to do anything with the error exception object, instead of doing this:

begin try_something rescue => e do_nothing_with_e end

You can drop the error exception object:

begin try_something rescue do_something end

And then finally, you can fold it into one line:

try_something rescue do_something

I don't advise using this in the majority of cases. I only used it because this was a test (not production code) and brevity helped keep things on one line.

Specifically, there is this odd quirk about JRuby where it calls the encoding constant UTF8 instead of UTF_8, so instead of me writing something very long and elaborate to check it such as:

if Encoding.constants.include?(:UTF_8) # do something with Encoding::UTF_8 elsif Encoding.constants.include?(:UTF8) # do something with Encoding::UTF8 end I just shortened it knowing that if you call `Encoding::UTF_8` in JRuby, it bombs, and then the `rescue` keyword rescues it unto the next value to compare to, which is `Encoding::UTF8` Again, this is not a recommended technique in general. I just did it because it was low-risk code in a test (spec).

jcouball · 2020-09-06T00:19:12Z

tests/units/test_diff_non_default_encoding.rb

    assert(patch.include?("-Φθγητ οπορτερε ιν ιδεριντ\n"))
    assert(patch.include?("+Φεθγιατ θρβανιτασ ρεπριμιqθε\n"))
  end

  def test_diff_with_japanese_and_korean_encoding
    d = @git.diff.path('test2.txt')
    patch = d.patch
+    return unless Encoding.default_external == (Encoding::UTF_8 rescue Encoding::UTF8) # skip test on Windows / check UTF8 in JRuby instead


Signed-off-by: Andy Maleh <andy.am@gmail.com>

AndyObtiva · 2020-10-25T08:57:24Z

Hi, it's been a month since I made the changes. I was wondering if there is anything else you need of me before merging this pull request.

I have been relying on my own fork (git-glimmer) for my project Glimmer DSL for SWT in the meantime.

I was hoping I would move back to the git gem trunk soon though.

Cheers,

Andy

p.s. Regarding skip 'Encoding tests do not work in Windows' if windows_platform?, I thought it was just a low-priority aesthetic thing that is just a matter of opinion and you could make the change yourself after merging. If you insist I make it instead, I'd be happy to if that is the hold up.

AndyObtiva force-pushed the master branch from b6f1b45 to bbe628c Compare August 6, 2020 04:59

jcouball reviewed Aug 16, 2020

View reviewed changes

AndyObtiva force-pushed the master branch 8 times, most recently from 1bc69ac to 63a5193 Compare August 17, 2020 15:41

jcouball added the Bug label Aug 17, 2020

jcouball modified the milestones: 1.7.0, 1.8.0 Aug 17, 2020

AndyObtiva force-pushed the master branch 12 times, most recently from d83c645 to 6194bdb Compare August 19, 2020 11:45

AndyObtiva force-pushed the master branch 5 times, most recently from 71af01d to 79b1ed0 Compare August 19, 2020 14:35

jcouball requested changes Sep 6, 2020

View reviewed changes

AndyObtiva force-pushed the master branch from 79b1ed0 to 53d7658 Compare October 3, 2020 17:52

Windows/JRuby fixes/tests/refactorings/travis-ci

f01715c

Signed-off-by: Andy Maleh <andy.am@gmail.com>

AndyObtiva force-pushed the master branch from 53d7658 to f01715c Compare October 3, 2020 18:22

AndyObtiva requested a review from jcouball October 3, 2020 18:29

jcouball approved these changes Oct 25, 2020

View reviewed changes

jcouball merged commit 1b5256c into ruby-git:master Oct 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

JRuby on Windows #480

JRuby on Windows #480

Uh oh!

AndyObtiva commented Aug 6, 2020 •

edited

Loading

Uh oh!

jcouball left a comment

Uh oh!

jcouball Aug 16, 2020 •

edited

Loading

Uh oh!

AndyObtiva Aug 17, 2020 •

edited

Loading

Uh oh!

jcouball commented Aug 16, 2020

Uh oh!

AndyObtiva commented Aug 17, 2020 •

edited

Loading

Uh oh!

jcouball commented Aug 17, 2020

Uh oh!

jcouball commented Aug 17, 2020

Uh oh!

AndyObtiva commented Aug 19, 2020 •

edited

Loading

Uh oh!

jcouball left a comment

Uh oh!

jcouball Sep 6, 2020

Uh oh!

jcouball Sep 6, 2020

Uh oh!

AndyObtiva Sep 25, 2020

Uh oh!

jcouball Sep 6, 2020

Uh oh!

AndyObtiva commented Oct 25, 2020

Uh oh!

Uh oh!

JRuby on Windows #480

JRuby on Windows #480

Uh oh!

Conversation

AndyObtiva commented Aug 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Your checklist for this pull request

Description

Uh oh!

jcouball left a comment

Choose a reason for hiding this comment

Uh oh!

jcouball Aug 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyObtiva Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouball commented Aug 16, 2020

Uh oh!

AndyObtiva commented Aug 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouball commented Aug 17, 2020

Uh oh!

jcouball commented Aug 17, 2020

Uh oh!

AndyObtiva commented Aug 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouball left a comment

Choose a reason for hiding this comment

Uh oh!

jcouball Sep 6, 2020

Choose a reason for hiding this comment

Uh oh!

jcouball Sep 6, 2020

Choose a reason for hiding this comment

Uh oh!

AndyObtiva Sep 25, 2020

Choose a reason for hiding this comment

Uh oh!

jcouball Sep 6, 2020

Choose a reason for hiding this comment

Uh oh!

AndyObtiva commented Oct 25, 2020

Uh oh!

Uh oh!

AndyObtiva commented Aug 6, 2020 •

edited

Loading

jcouball Aug 16, 2020 •

edited

Loading

AndyObtiva Aug 17, 2020 •

edited

Loading

AndyObtiva commented Aug 17, 2020 •

edited

Loading

AndyObtiva commented Aug 19, 2020 •

edited

Loading