CDK-R / cdkr

Integrating R and the CDK
https://cdk-r.github.io/cdkr/
42 stars 27 forks source link

Major Changes to JDK17 #126

Closed zachcp closed 2 years ago

zachcp commented 2 years ago

I received the following from Brian Ripley:

Java 17 was released this week: as it is the first version officially
supporting arm64 Macs it is being used for the 'M1mac' additional
issues.  Packages

Myrrix RJDemetra RMOA expands jdx jsr223 rcdk rjdmarkdown tabulizer

fail, and this has been confirmed under Linux.  Pre-built binary
packages are already available from https://jdk.java.net/17/ and will
shortly be available from https://adoptium.net/.  As 17 is an LTS
release it is expected to become widely used over the next few months.

On the most common form of failure (all but jsr223, also seen in rJava),
Simon Urbanek writes

"There was a major change in JDK-17 that now disallows reflection to use
non-public access.  In short rJava can no longer pretend it is part of a
class when accessing methods/fields which allowed us to call methods
without explicitly subclassing a class.  This may have serious
repercussions, because code that could be before written in R now has to
be written in Java, compiled and then called from R."

Please correct before 2021-10-17 to safely retain your package on CRAN.

I can try to look into this over the weekend. Much of the rCDK is compiled so I don't think the affected surface area will be too bad but I expect the use of $ for convenient reflection will be severely curtailed.

zach cp

zachcp commented 2 years ago

Additional Info from the rJava Changelog.

1.0-5   (under development)
    o   allow access modifications to fail on JDK-17

    == Important note to Java developers:
    JDK-17 no longer allows access overrides, so J() wrapper
    will fail for cases where the underlying class does
    not allow public access. This means that it is no longer
    possible to write high-level code that behaves as if it was a
    member of the class. Do not upgrade to JDK-17 if that is
    important in your application or write a Java wrapper that
    exposes the necessary fields/methods as public.
zachcp commented 2 years ago

I did a bit of work on this and ran into the classic "setting up a full R+Java dev environment" and got stuck. However, since provisioning in CI is straightforward I setup up GH actions to use JDK17 and run the tests (from this branch). I tried to avoid the use of reflection by getting rid of $ but we're still triggering errors... so there may be deeper issues in the code.

@rajarshi there are only 2 weeks left to submit to updated packages to CRAN without risking package removal. Any chance you can also take a crack at this?

zachcp commented 2 years ago

Addressed in #127

zachcp commented 2 years ago

A new issue arising for CRAN submission. Checks are finding tests CPU/real time ratio is off. Are some of the underlying JAVA methods defaulting to multicore?

...
* checking examples ... [4s/2s] NOTE
Examples with CPU time > 2.5 times elapsed time
                 user system elapsed ratio
get.fingerprint 0.934   0.08   0.198 5.121
* checking for unstated dependencies in ‘tests’ ... OK
* checking tests ... [8s/2s] OK
  Running ‘doRUnit.R’ [8s/2s]
Running R code in ‘doRUnit.R’ had CPU time 3.6 times elapsed time
* checking for unstated dependencies in vignettes ... OK
* checking package vignettes in ‘inst/doc’ ... OK
* checking re-building of vignette outputs ... [10s/5s] OK
* checking PDF version of manual ... OK
* ...
rajarshi commented 2 years ago

Hmm, that shouldn't be as CDK itself is not threadsafe

On Fri, Oct 15, 2021 at 9:15 AM zachcp @.***> wrote:

A new issue arising for CRAN submission. Checks are finding tests CPU/real time ratio is off. Are some of the underlying JAVA methods defaulting to multicore?

...

  • checking examples ... [4s/2s] NOTE

Examples with CPU time > 2.5 times elapsed time

             user system elapsed ratio

get.fingerprint 0.934 0.08 0.198 5.121

  • checking for unstated dependencies in ‘tests’ ... OK

  • checking tests ... [8s/2s] OK

    Running ‘doRUnit.R’ [8s/2s]

Running R code in ‘doRUnit.R’ had CPU time 3.6 times elapsed time

  • checking for unstated dependencies in vignettes ... OK

  • checking package vignettes in ‘inst/doc’ ... OK

  • checking re-building of vignette outputs ... [10s/5s] OK

  • checking PDF version of manual ... OK

  • ...

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/CDK-R/cdkr/issues/126#issuecomment-944289934, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAIMOPTFMKGISYA5SEGN4DUHASQ3ANCNFSM5EOY4ZBA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

-- Rajarshi Guha | http://blog.rguha.net | @rguha https://twitter.com/rguha

zachcp commented 2 years ago

addressed in #127

zachcp commented 2 years ago

Package on the way to CRAN. @rajarshi, once you merge #127 can you tag a release for 3.6.0?