Closed rafapereirabr closed 5 months ago
Hi @rafapereirabr , thank you for testing and providing feedback!
I wanted to be very cautious about changing system-wide settings, especially such critical ones as JAVA_HOME
and PATH
. I agree that it is worth implementing the ability to set these globally (apparently pointing to the cache in the package directory, not in the project directory), but I also strongly believe, as you suggested, that this feature should be off by default and paths should be set at project level.
Additionally, there should probably be a mechanism to back up the current system-wide path variables and a function to restore them if something goes wrong.
Hi Egor, I agree with wverything you said. And yes, it is perfectly reasonable to set local paths attached to each project as the default behavior of the function. If it would be possible to set a global path saved in a cache dir of the package, it would be fantastic though.
@rafapereirabr
I have reworked env set/unset and quick install and install functions to implement global installation of Java for all R sessions. I did not implement it exactly as you suggested. The associated pull request is here #7 - I hope it may help overview the under the hood changes if you want to have a look.
To test:
remotes::install_github("e-kotov/rJavaEnv@global-java-home-path", force = T)
Updated behaviour is as follows.
You can now install Java locally in the current project or globally for all projects.
This code will install globally by adding to the global .Rprofile
file in the user's home directory. We need "session" to immediately set this path in the current session too so that we can use it right away.
library(rJavaEnv)
java_quick_install(8, where = c("session", "global"))
You can of course just do:
java_quick_install(8, where = "global")
But Java will not work until you manually java_set_env()
to the Java binaries quick installed in the cache folder of rJavaEnv... So in a way the full java_quick_install(8, where = c("session", "global"))
is also a safety mechanism that prevents breaking things, though looks more complicated than you suggested.
To revert:
java_env_unset('global')
To install Java in the current project, as before you can
java_quick_install(8)
and it defaults to "project" and "session".
To unset:
java_env_unset('project')
Perhaps this is a bit too cumbersome... Maybe it is worth keeping the flexibility of java_env_set()
to accept combinations of "project", "global" and "session", but for java_quick_install()
it would be enough to just use either "project" or "global" and avoid "session" value for the "where" argument, and just always set the newly quick installed environment in the current session, as it is logical that the user want to use it right away. But for java_install()
and java_env_set()
I would keep the flexibility and defaults as is, because they can potentially be used to install and set multiple java versions independently in R subprocesses, for example in workers spawned by targets
.
Let me know what you think of this design. I am not merging the pull request just yet and ready to rework the branch again.
UPDATED:
As it turned out, the backup of env variables is not needed, as they are restored as soon as we clear the rJavaEnv's additions to the global .Rprofile
in user's home directory and restart the R session.
This all sounds reasonable to me. Just to clarify, after running this:
# check version of JDK currently installed (if any)
rJavaEnv::java_check_version_rjava()
# install JDK 21
rJavaEnv::java_quick_install(
version = 21,
distribution = 'Corretto')
the user will also need to run this?
java_set_env()
Just wanna make sure I have this right for the workshop this Monday (June 17)
@rafapereirabr
At the moment this:
# check version of JDK currently installed (if any)
rJavaEnv::java_check_version_rjava()
# install JDK 21
rJavaEnv::java_quick_install(
version = 21,
distribution = 'Corretto')
will install and set the environment with no additional action required. I would add another rJavaEnv::java_check_version_rjava()
after quick install to verify that the Java was successfully installed. And this will still work the in the same way if I merge the pull request that addresses your request into main.
If I merge the pull request as it is right now, you can also instruct (or give option) to the participants to use the new parameter and do:
rJavaEnv::java_quick_install(
version = 21,
distribution = 'Corretto',
where = c("session", "global"))
to install globally, as you requested in the issue, and also set it in the current session immediately. In any R session they have from now on they will have the same installed Java JDK version in the path. After that no additional action is required, except for rJavaEnv::java_check_version_rjava()
again to check that correct Java is available.
p.s. funny, I noticed the workshop on Twitter about 30 minutes before you posted the comment above.
Thanks for the clarification, @e-kotov !
@rafapereirabr ok, I take it you want to focus you the workshop for now. I will not be merging the changes just yet, so that your workshop goes well and is not interrupted by the changes in the package (even though I designed them in the way that the code you are using in the workshop will still work).
For now, I'm thinking that java_quick_install()
, as a convenience command for people who just want the quick result with reasonable defaults should always set the installed version of Java in the current session in addition to saving the settings project or globally. It should act as follows:
.Rprofile
in the current project) and immediately apply in the current session:java_quick_install(21) # with project being default
Or more explicitly:
java_quick_install(21, where = "project")
.Profile
in the user home directory) and immediately apply in the current session:java_quick_install(21, where = "global")
java_quick_install(21, where = "global", current_session = FALSE)
But in general, if there is a good reason why the newly installed Java should not be set in the current session (e.g. a script that just installs several java versions in the same project, one for {r5r}
, one for {opentripplanner}
), a user should use java_install()
with autoset_java_path = FALSE
and then use java_env_set(java_home, where = 'session')
in the relevant R scripts that will using {r5r}
or {opentripplanner}
(or other Java/rJava dependent R package).
Thinking out loud below.
Looking back at the current code, because I did not think about the usefulness of setting Java globally outside the current project, I will need to rethink the naming of the arguments of java_quick_install
and java_install
and unify them. I don't like the current autoset_java_path
of java_install
as it does not give the freedom to choose between project, global and session but only allows to turn off the autoset of environment in both session and project at the same time.
Hi @e-kotov . Thanks for all this. The workshop was very successful with many attendees and almost everyone of them were able reproduce all the code of the workshop. The {rJavaEnv}
as really really important. Installing Java is the most "difficult" part of learning {r5r}
and many people did not have Java installed. {rJavaEnv}
was super handy! There were only two or three people who couldn't get Java installed with {rJavaEnv}
, but I couldn't really figure out the reason.
In short, thanks a bunch. Please let me know if I you need any feedback on recent developments of {rJavaEnv}
and I'd glad to help
Hi @rafapereirabr,
That's exciting news! I'm thrilled that it worked for most people in your workshop!
To give me an idea of the success rate, how many people attended the workshop?
Do you, by any chance, remember which operating system was installed on the computers of those who had issues? I noticed there was a person with an Intel-based Mac (you already saw this issue here: https://github.com/ipeaGIT/r5r/issues/393#), and there may also be some problems on Linux (discussed in detail in issues #3 and #8).
Regarding the feature of setting JAVA_HOME globally, as mentioned in the discussion of issue #3, one of the core R developers noted that editing the global .Rprofile
is not only a bad idea (which I agree with and is the reason I made the default to edit the project/current directory .Rprofile
only) but is also not allowed on CRAN. Therefore, if we want {rJavaEnv}
to be on CRAN someday, it's best not to implement this and stick to the project level, just like {renv}
does.
I think at this moment {rJavaEnv}
has all the minimal features in place. The current short-term development goal is mostly internal changes such as minor code cleanup and test coverage, as well as addressing issue #3 on Linux.
The next step is to include other Java distributions in addition to Amazon Corretto. This is relatively simple, as it only requires editing a JSON file. However, this does not seem to be a priority, as things work just fine with Corretto, at least for {r5r}
, which is currently the top "client" of {rJavaEnv}
😉.
For now, no help is needed from your side. I am subscribed to Issues in {r5r}
, so I am keeping an eye on possible Java-related issues.
I think we had aprox. 35 to 40 attendees in the workshop. Among those who had issues installing Java using {rJavaEnv}
, I remember there was one person using a Mac and another one on Windows.
I totally understand if it would not be possible to set JAVA_HOME path globally due to CRAN policies. In such case, it's best to set the path within projects or R sessions.
@rafapereirabr wow, up to 40 attendees is a huge workshop!
Hi @e-kotov , thanks for creating
{rJavaEnv}
! I tried the package and it was so simple and quick to use. A great contribution to the R community.I noticed that the
java_quick_install()
functions sets a Java version in current working/project directory. Would it be possible to set a Java version globally?I thought of perhaps including a parameter like
java_quick_install(local = c(TRUE, FALSE))
. This way, the user could choose to set a java version locally to a particular working/project directory (local = TRUE
), OR to set a global version that would work for every project (local = FALSE
). In the latter case, the advantage is that the user would need to call thejava_quick_install()
function only once.