Skip to content

Conversation

@ahorek
Copy link

@ahorek ahorek commented Jun 12, 2021

implements #20

@ahorek
Copy link
Author

ahorek commented Jun 13, 2021

I've fixed false positive failures with assert_separately because any warning is an error https://github.com/jruby/jruby/blob/master/core/src/main/java/org/jruby/Ruby.java#L3089

@olleolleolle olleolleolle linked an issue Jun 13, 2021 that may be closed by this pull request
@hsbt
Copy link
Member

hsbt commented Jun 14, 2021

@knu Can you review this? I prefer to merge JRuby implementation to this repository.

Copy link
Member

@knu knu left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, I think this is the way to go!

digest.gemspec Outdated
ext/digest/sha2/extconf.rb
]

if RUBY_ENGINE == 'jruby'
Copy link
Member

Choose a reason for hiding this comment

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

Use Gem::Platform for --platform option of gem build.

Suggested change
if RUBY_ENGINE == 'jruby'
if Gem::Platform === spec.platform and spec.platform =~ 'java'

Copy link
Author

Choose a reason for hiding this comment

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

digest.gemspec Outdated
Comment on lines 17 to 22
"ext/digest/bubblebabble/bubblebabble.c", "ext/digest/bubblebabble/extconf.rb", "ext/digest/defs.h",
"ext/digest/bubblebabble/bubblebabble.c", "ext/digest/bubblebabble/extconf.rb", "ext/digest/bubblebabble/lib/bubblebabble.rb", "ext/digest/defs.h",
"ext/digest/digest.c", "ext/digest/digest.h", "ext/digest/digest_conf.rb", "ext/digest/extconf.rb",
"ext/digest/md5/extconf.rb", "ext/digest/md5/md5.c", "ext/digest/md5/md5.h", "ext/digest/md5/md5cc.h",
"ext/digest/md5/md5init.c", "ext/digest/rmd160/extconf.rb", "ext/digest/rmd160/rmd160.c",
"ext/digest/rmd160/rmd160.h", "ext/digest/rmd160/rmd160init.c", "ext/digest/sha1/extconf.rb",
"ext/digest/sha1/sha1.c", "ext/digest/sha1/sha1.h", "ext/digest/sha1/sha1cc.h",
"ext/digest/md5/md5init.c", "ext/digest/md5/lib/md5.rb", "ext/digest/rmd160/extconf.rb", "ext/digest/rmd160/rmd160.c",
"ext/digest/rmd160/rmd160.h", "ext/digest/rmd160/rmd160init.c", "ext/digest/rmd160/lib/rmd160.rb", "ext/digest/sha1/extconf.rb",
"ext/digest/sha1/sha1.c", "ext/digest/sha1/sha1.h", "ext/digest/sha1/sha1cc.h", "ext/digest/sha1/lib/sha1.rb",
Copy link
Member

Choose a reason for hiding this comment

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

These added files should be inside if RUBY_ENGINE == 'jruby' block.

Copy link
Author

Choose a reason for hiding this comment

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

java sources don't have to be part of a released gem, so I removed them instead

@ahorek ahorek force-pushed the jruby_support branch 2 times, most recently from 60bfe18 to b0cdf61 Compare June 14, 2021 11:52
@rhenium
Copy link
Member

rhenium commented Jun 14, 2021

JRuby's digest is licensed under EPL 2.0/GPL 2.0/LGPL 2.1 unlike MRI's. To my understanding it can't simply be relicensed under Ruby's/2-clause BSDL, so that has to be reflected in the licenses field of the gemspec for JRuby builds.

I feel it's also worth mentioning in README that files under ext/java have a different license from others.

@hsbt
Copy link
Member

hsbt commented Jun 14, 2021

Can we switch the license in gemspec each platforms?

@ahorek ahorek force-pushed the jruby_support branch 2 times, most recently from 0df8176 to dbc83db Compare June 14, 2021 12:35
@ahorek
Copy link
Author

ahorek commented Jun 14, 2021

Can we switch the license in gemspec each platforms?

done!

@ahorek ahorek requested a review from nobu June 14, 2021 20:34
@headius
Copy link

headius commented Jun 14, 2021

I will review quick as well but looks most of the issues have been addressed.

If it would be better for us to relicense we can try to contact past contributors and get permission, but it would take some time.

@headius
Copy link

headius commented Jun 14, 2021

Committers to the Digest library in JRuby, as far back as 2006, before which only Ola contributed to it:

Charles Oliver Nutter
Ola Bini
Thomas Enebo
Andrey Fadeyev
Chris Heald
Chris Seaton
kares
tduehr

Copy link

@headius headius left a comment

Choose a reason for hiding this comment

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

Mostly build plumbing but the changes look fine to me.

@headius
Copy link

headius commented Jun 14, 2021

@ahorek Have you tested this gem with a build of JRuby that does not include our built-in version? If that works then I think this is ready to go.

@headius
Copy link

headius commented Jun 14, 2021

Those JRuby digest contribs in GH user form:

@ahorek
Copy link
Author

ahorek commented Jun 14, 2021

@headius yes, I tested it together with jruby/jruby#6719

standard build

rake compile
gem build

jruby build

rake compile:java
gem build --platform java

as suggested by @nobu we can build a Java version directly from CRuby, but it doesn't work the opposite way, on JRuby the Java platform is forced, so you won't be able to build a version for CRuby on JRuby.

@cheald
Copy link

cheald commented Jun 14, 2021

I happily grant permission to relicense code I contributed.

@headius
Copy link

headius commented Jun 30, 2021

I think we should go ahead and merge this with the license disclaimer. If we really want to relicense open a separate issue and ping the folks I mentioned above for permission. Since only one replied here, it may take a little work to chase everyone down.

@tduehr
Copy link

tduehr commented Jun 30, 2021

I'm fine with re-licensing all code I contributed to JRuby to Ruby's/2-clause BSDL.

@kares
Copy link

kares commented Jul 1, 2021

👍 I consent to my contributions being relicensed under the Ruby license and the BSD 2-clause license.

@enebo
Copy link

enebo commented Jul 1, 2021

I am fine with relicensing all code to Ruby's/2-clause BSDL.

@headius
Copy link

headius commented Jul 1, 2021

I consent to my contributions being relicensed under the Ruby license and the BSD 2-clause license.

@kubum
Copy link

kubum commented Jul 1, 2021

Sorry for not coming back earlier. I am happy for this code to be in. The fragment I added in 2015 as part of the jruby/jruby#3238 PR, in fact, is just a mere port of an else branch from OpenSSH sshkey.c file written Java. The documentation header already points to it.

@olabiniV2
Copy link

Hi everyone. I am Ola Bini - the owner of the @olabini account - which I'm currently locked out of. If any legal issues arise, I'm willing to provide offline government identification if necessary, to support this agreement.

I consent to my contributions being relicensed under the Ruby license and the BSD 2-clause license.

@chrisseaton
Copy link

Happy.

@nobu
Copy link
Member

nobu commented Sep 6, 2021

I think lib/digest.rb should move back to ext/digest/lib/, like as other .rb files under ext/digest/*/lib directories.
And jruby doesn’t have similar feature to copy incidental scripts?

@knu
Copy link
Member

knu commented Sep 6, 2021

I think lib/digest.rb should move back to ext/digest/lib/, like as other .rb files under ext/digest/*/lib directories.
And jruby doesn’t have similar feature to copy incidental scripts?

The autoloading stuff should be moved there, but common parts can stay there.

@knu
Copy link
Member

knu commented Sep 6, 2021

But I think I can and should do that myself, because I'll need to modify the import script (to ruby/ruby) anyway.

@nobu
Copy link
Member

nobu commented Sep 7, 2021

My proposal is to make JRuby libraries into a separate directory from the libraries under ext/digest.
https://github.com/nobu/digest/tree/jruby-support

@ahorek
Copy link
Author

ahorek commented Sep 7, 2021

I agree with your proposal @nobu thanks for your help.

@headius
Copy link

headius commented Sep 7, 2021

And jruby doesn’t have similar feature to copy incidental scripts?

JRuby does not have a separate source dir for .rb files associated with extensions. If /ext/digest/lib is a better place and it is added to the load path for the gem, we should be able to find it. We might need to enhance our default gem installation logic to copy those files to the common stdlib dir, but that's for us to fix.

@nobu
Copy link
Member

nobu commented Sep 13, 2021

JRuby does not have a separate source dir for .rb files associated with extensions. If /ext/digest/lib is a better place and it is added to the load path for the gem, we should be able to find it.

That directory is for scripts to be installed by extconf.rb and make.
What I was thinking isn’t to add that directory to the load path, but is the same as Eregon’s suggestion (#21 (comment)).

@knu knu self-assigned this Sep 14, 2021
@knu
Copy link
Member

knu commented Sep 14, 2021

OK, I'll take this! Thanks to all the contributors who agreed with the license change!

@ahorek
Copy link
Author

ahorek commented Sep 23, 2021

I've merged Nobu's changes and rebased against the current master. I think it should be ready.

@knu knu mentioned this pull request Sep 28, 2021
@knu
Copy link
Member

knu commented Sep 28, 2021

I just pushed #25 which should include the core parts of this PR while avoiding dynamic library file generation. Please look into it and leave any comments.

@knu
Copy link
Member

knu commented Sep 30, 2021

Thank you all for your cooperation. This work has been merged, and I'll be working on a new gem release shortly.

@headius
Copy link

headius commented Sep 30, 2021

@knu Thank you! If possible, could you push a prerelease gem that we can run through JRuby CI, to ensure it is working properly?

@knu
Copy link
Member

knu commented Sep 30, 2021

@ahorek
Copy link
Author

ahorek commented Sep 30, 2021

@knu there's a missing jar file, see #27 could you take a look and try it one more time? thanks!

@knu
Copy link
Member

knu commented Sep 30, 2021

@knu
Copy link
Member

knu commented Sep 30, 2021

Argh, rake release doesn't seem to run rake compile and that's why pre1 still doesn't seem to include the jar file. I'll release another pre.

@knu
Copy link
Member

knu commented Sep 30, 2021

@headius
Copy link

headius commented Sep 30, 2021

@knu Thank goodness for prerelease gems! I will test this on our 3.0 dev branch now.

@headius
Copy link

headius commented Sep 30, 2021

Looking good so far but I need to modify our build to copy the ext lib files for JRuby into our standard lib directory. Once I know everything is installing properly, I will let you know.

@headius
Copy link

headius commented Sep 30, 2021

@knu I believe it is working! You may go ahead and release 3.1.0.

$ spec/mspec/bin/mspec ci spec/ruby/library/digest/
$ /Users/headius/projects/jruby/bin/jruby /Users/headius/projects/jruby/spec/mspec/bin/mspec-ci spec/ruby/library/digest/
jruby 9.4.0.0-SNAPSHOT (3.0.2) 2021-09-30 5039c194de OpenJDK 64-Bit Server VM 11.0.4+11 on 11.0.4+11 +jit [darwin-x86_64]
[/ | ==================100%================== | 00:00:00]      0F      0E 

Finished in 3.434759 seconds

68 files, 129 examples, 230 expectations, 0 failures, 0 errors, 5 tagged

@knu
Copy link
Member

knu commented Oct 1, 2021

@headius Thank you!

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.

java version