adds compatibility to install gems with jruby > 9.2.10.x #106

abelsromero · 2020-05-24T19:44:56Z

This PR fixes #105 by checking the Ruby language version. If 2.6 or higher, it will replace the rdoc and ri arguments (noew deprecated) by the supported document. Older versions of Ruby runtimes will work same as now.

Only thing I am not confident is that when running mvn clean test -pl gem-maven-plugin -am I get this error gem-maven-plugin: Execution default of goal org.apache.maven.plugins:maven-plugin-plugin:3.4:helpmojo failed: The source must not be a directory. Any idea?

It also:

Adds Idea files to .gitignore
Fixes artifactId in rubygems-in-test-resources-failure test. This caused false positives when running all IT tests (those run by maven-invoker-plugin) in a specific order.
Fixes test include-rubygems-in-test-resources adding the correct dependency. This worked due to previoud comment, but failed when run in isolation.

abelsromero · 2020-05-24T20:02:12Z

I see the project fails because Travis is using Java 11 which fails. Not sure this is blocking. I could make the project build and pass all test with Java8, but going higher requires more work.

After this PR we could discuss on "up-to-date" roadmap? At least make it so the project is build with Java11 but is still compatible with Java 8 if necessary.

abelsromero · 2020-06-03T10:04:26Z

@mkristian any comment?

donv · 2020-06-22T08:12:36Z

We also need this change.

kares · 2020-07-09T14:32:31Z

ruby-tools/src/main/java/de/saumya/mojo/ruby/gems/GemsInstaller.java

+        try {
+            return this.factory.getVersion();
+        } catch (Exception e) {
+            return null;


do we really need to swallow silently?

I would argue a not aborting with hard failure here is not the end of the world...but...

It should warn that it cannot detect version (I think what @kares wants?)

It should not emit no rdoci/ri since it really does not know what jruby version it is (and therefore will work for all versions of jruby).

You are right @enebo, in some way the idea was to keep it as similar as possible with current behauviour. So if for some reason the version could not be extracted*, I prefered a more "optimistic" approach and fall back to current behauviour. The code is 1.5 so could not use Optional which may had been nicer.
About the warning is not so easy, I can inject the Logger from the mojo when called from AbstractGemMojo, but not when called from RailsService. To avoid extra checks I could use Slf4j if it's ok, Maven picks it up. or just create a Slf4j if none is injected.

*Note we would need to parse several differents formats for differents Ruby implementions and for now I focused only on jruby.

@abelsromero Sounds reasonable to me.

After a second thought I think it doesn't make sense to treat that in GemInstaller. I move it to ScriptFactory and added a WARN message there.
wdyt?

olleolleolle

Thanks for building on this!

I added inline reader's notes, all of which are from a Ruby person's perspective, with uneasy Java fluency.

olleolleolle · 2020-07-15T07:31:11Z

gem-maven-plugin/src/it/install-gem-ruby-9.2.10-compatibility/pom.xml

+  <groupId>rubygems</groupId>
+  <artifactId>pom_first</artifactId>
+  <version>0.0.0</version>
+  <description>Installs a gem from http://rubygems.org with Ruby 2.7 compatibility</description>


Here is a line which made me wonder: http vs https?

It's not recommended to request gems via http from rubygems.org.

Is there a special reason to not use https in this PR?

Only reason is legacy configuration. Right now it shows a warning but works, and I see there're other places where http is used that are not Java so not sure of the impact.
I am happy to work on it but I'd rather do it in a separated issue. wdyt?

Separate issue, totally right decision, and it's not a priority for me. Thanks for answering!

olleolleolle · 2020-07-15T07:31:54Z

gem-maven-plugin/src/it/install-gem-ruby-9.2.10-compatibility/verify.bsh

+File f = new File( basedir, "target/rubygems/gems/yard-0.9.25" );
+if ( !f.exists() )
+{
+    throw new RuntimeException( "file does not exists: " + f );


Grammar note:

Suggested change

throw new RuntimeException( "file does not exists: " + f );

throw new RuntimeException( "file does not exist: " + f );

👍 I will also check the other tests, I recall copying it.

olleolleolle · 2020-07-15T07:44:44Z

ruby-tools/src/main/java/de/saumya/mojo/ruby/script/JRubyVersion.java

+            return true;
+
+        if (minor < Integer.parseInt(parts[1]))
+            return true;


I wonder here, the minor and parts[1] seem to be compared in separation of major.

Does this mean that 2.5.0 would be considered lower than 1.9.2?

Perhaps I'm just confused, that could also be it.

Just added that case to a test and you are right. I will fix and add more extra tests to JRubyVersionTest to ensure all cases.

…ons in version reporting

abelsromero · 2020-07-16T23:01:03Z

Thanks to @olleolleolle's comment I fount the approach was not reliable because jruby always reports 2.5.7 and my test where working due a false positive. I tried other options and asked int the jRuby Matrix channel and no solutions was available 😞 At the end I check the explicit jRuby version.

Also, today I check what jruby-gralde-plugin is doing and they just use --no-document, so maybe we could resolt to the same if current appoach causes issues. I could not find when this was introduced, but there are some references of more than 5 years.

@enebo @olleolleolle @kares PR ready for final review 🚀

olleolleolle · 2020-07-20T06:41:22Z

I filed #108 to remember the JDK11 issue.

abelsromero · 2020-07-24T07:02:39Z

Any comment?

headius · 2020-07-28T21:40:35Z

I will review soon!

donv · 2020-08-19T09:08:10Z

Any progress?

donv · 2020-09-10T06:26:50Z

@headius Any progress on this one? 😃

headius · 2020-09-16T20:41:36Z

Sorry this got left behind. Looking into it now.

headius · 2020-09-16T20:50:05Z

The tests are failing on Travis because they run with Java 11 and use JRuby 1.7.3 for certain actions, and "11" is rejected as a version number by JRuby 1.7.3. I am testing locally to confirm things are working, and then will fix the use of this very old JRuby.

headius · 2020-09-16T22:46:31Z

The branch appears to fail for a different reason locally, but master also fails for the same reason. So the tests are a bit out of date (like most of this library) and I think we can go ahead with merging this.

headius · 2020-09-16T22:56:14Z

This PR was a bit messy to review, due to the many whitespace and formatting changes throughout. I am guessing some of these were done automatically by a tool or an IDE, which is difficult to avoid. The changes appear to align the associated files with JRuby standards for source formatting, so I will still proceed with the merge.

There's also a spelling correction of "exists" to "exist" throughout that made the diff much larger than it needed to be.

In the future, please keep whitespace, spelling, and documentation changes in their own PRs if they are not directly related to the PR's primary changes.

And finally, thank you for putting this together and being so patient about it being merged! It has been a crazy summer.

@mkristian I will merge this, but I can't remember if I have Maven publishing rights or not. Either way we will want to do a release...possibly after fixing #107 if it is not already.

abelsromero · 2020-09-17T07:10:33Z

This PR was a bit messy to review, due to the many whitespace and formatting changes throughout. I am guessing some of these were done automatically by a tool or an IDE, which is difficult to avoid. The changes appear to align the associated files with JRuby standards for source formatting, so I will still proceed with the merge.

Spot on! I was aware of that and I checked if there was any formatting configuration in the docs, but saw nothing, and trying to keep the current formatting manually was impossible and extremely unproductive.

In the future, please keep whitespace, spelling, and documentation changes in their own PRs if they are not directly related to the PR's primary changes.

If so, formatting configuration should be provided in the project. If there's any guide, point me and I can look into it. Otherwise, for the sake of lowering the barrier of entry I would stick with whatever most popular IDE provides ootb.
Also, side note, eclipse formating XML (most popular afaik) does not tell anything about import order and has things that some IDEs do not recognize, I have had issues with that also in the past :/ nothing is perfect

headius · 2020-09-17T15:25:24Z

@abelsromero Yeah I understand the difficulty, especially since the sources in this project were pretty far from typical Java formatting ("else" and friends on new line, tabs instead of spaces in some places, etc). In general the standard is really just typical Java formatting guidelines. IDEs may vary, but I don't believe any will automatically use the formatting that existed here.

headius · 2020-09-17T15:26:26Z

Actually I think it wasn't tabs as much as useless whitespace on newlines that got cleaned up, but again I think most IDEs would deal with that better.

We will try to format things better going forward. If your IDE happens to "fix" another file, don't worry about it too much.

adds compatibility to install gems with jruby > 9.2.10.x

1c6b906

kares reviewed Jul 9, 2020

View reviewed changes

refactor to handle jruby version discovery errors in ScriptFactory

55f8be4

olleolleolle reviewed Jul 15, 2020

View reviewed changes

abelsromero changed the title ~~adds compatibility to install gems with jruby > 9.2.10.x~~ WIP: adds compatibility to install gems with jruby > 9.2.10.x Jul 15, 2020

abelsromero added 4 commits July 16, 2020 21:12

fix JRubyVersion language comparison

23511c6

fix 'file does not exists' typo in tests and RSpecMojo

b38682c

replace langauge version comparision by jruby version due to limitati…

2f20e77

…ons in version reporting

fix IT test execute-compass-with-gems-from-plugin

a9194eb

abelsromero changed the title ~~WIP: adds compatibility to install gems with jruby > 9.2.10.x~~ adds compatibility to install gems with jruby > 9.2.10.x Jul 16, 2020

headius merged commit b453266 into jruby:master Sep 16, 2020

This was referenced Jun 10, 2021

Drop jruby 9.1 support jruby/warbler#499

Merged

gem-maven-plugin uses the deprecated "--no-rdoc" command #116

Open

Release 2.1.0 jruby/warbler#498

Open

This was referenced Oct 28, 2021

Bump mvn extensions jruby/ruby-maven#2

Merged

Restore java 8 support #119

Merged

headius added this to the pre-3.0.0 milestone Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds compatibility to install gems with jruby > 9.2.10.x #106

adds compatibility to install gems with jruby > 9.2.10.x #106

abelsromero commented May 24, 2020

abelsromero commented May 24, 2020

abelsromero commented Jun 3, 2020

donv commented Jun 22, 2020

kares Jul 9, 2020 •

edited

Loading

enebo Jul 9, 2020

abelsromero Jul 9, 2020

enebo Jul 10, 2020

abelsromero Jul 11, 2020

olleolleolle left a comment

olleolleolle Jul 15, 2020

abelsromero Jul 15, 2020

olleolleolle Jul 15, 2020

olleolleolle Jul 15, 2020

abelsromero Jul 15, 2020

olleolleolle Jul 15, 2020

abelsromero Jul 15, 2020 •

edited

Loading

olleolleolle Jul 15, 2020

abelsromero commented Jul 16, 2020

olleolleolle commented Jul 20, 2020

abelsromero commented Jul 24, 2020

headius commented Jul 28, 2020

donv commented Aug 19, 2020

donv commented Sep 10, 2020

headius commented Sep 16, 2020

headius commented Sep 16, 2020

headius commented Sep 16, 2020

headius commented Sep 16, 2020

abelsromero commented Sep 17, 2020

headius commented Sep 17, 2020

headius commented Sep 17, 2020

	throw new RuntimeException( "file does not exists: " + f );
	throw new RuntimeException( "file does not exist: " + f );

adds compatibility to install gems with jruby > 9.2.10.x #106

adds compatibility to install gems with jruby > 9.2.10.x #106

Conversation

abelsromero commented May 24, 2020

abelsromero commented May 24, 2020

abelsromero commented Jun 3, 2020

donv commented Jun 22, 2020

kares Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olleolleolle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abelsromero Jul 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abelsromero commented Jul 16, 2020

olleolleolle commented Jul 20, 2020

abelsromero commented Jul 24, 2020

headius commented Jul 28, 2020

donv commented Aug 19, 2020

donv commented Sep 10, 2020

headius commented Sep 16, 2020

headius commented Sep 16, 2020

headius commented Sep 16, 2020

headius commented Sep 16, 2020

abelsromero commented Sep 17, 2020

headius commented Sep 17, 2020

headius commented Sep 17, 2020

kares Jul 9, 2020 •

edited

Loading

abelsromero Jul 15, 2020 •

edited

Loading