Skip to content

Conversation

@mattwigway
Copy link
Contributor

We've run into a number of issues with the auto-rebuild of the JAR in the past (e.g. version mismatches etc.), and it's generally not best practice to have compiled code checked into the source repository anyhow for a number of reasons. This is a proof-of-concept removing the JAR from the git repository and instead building it from scratch when R CMD build is executed. This won't affect the vast majority of users who get binary packages from CRAN, but will ensure that the R and java code are in sync.

I know @rafapereirabr has been hesitant to change the build system in the past, and I'm very sensitive to maintainability issues. I think this is actually more maintainable than the current Frankenstein Github Actions compile step that then checks the JAR into the repo, but I'll defer to him on whether this is a good direction to go, just wanted to put the proof of concept out there (hence the draft status). Definitely not something that should be targeted for r5r 2.4.0.

In order to make this work, the java code has to be in the r-package directory, so I moved it to the java directory, which is where CRAN wants it anyways. It may also make sense to get rid of the r-package directory entirely and just move its contents to the top level, since it no longer needs to be separated from the Java code.

The issue I am running into is that the package build requires devtools (because it runs devtools::load_all() so it can run download_r5() and get the correct R5 JAR. devtools is a huge package so I don't want to add it as a dependency, and it's only needed at build time, but I'm not sure how to specify that. The other CI failure is the same one discussed in #544.

@mattwigway
Copy link
Contributor Author

cc @alexmgns @e-kotov

@mattwigway
Copy link
Contributor Author

Not sure why Github isn't linking the CI results, but they're here

@mattwigway
Copy link
Contributor Author

To build during development, you just run devtools::build("r-package", binary = TRUE)

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.

1 participant