Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Install mlpack and cereal headers in R package #3488

Merged
merged 7 commits into from Jun 1, 2023

Conversation

rcurtin
Copy link
Member

@rcurtin rcurtin commented May 29, 2023

This handles #3486.

By putting the mlpack and cereal headers into inst/include/, they will be installed as part of the R package installation process. I also had to update Makevars to correctly include the headers (in the same way that's done in the Makevars of RcppEnsmallen).

@eddelbuettel
Copy link
Contributor

Just got in from 25 miles on the bicycle where I realized ... we are doing this likely wrong :) When you install the headers into src/, they should go directly to inst/include/ and src/Makevars should get a -I../inst/include to use them during package build. Anyway... Will review now.

Copy link
Contributor

@eddelbuettel eddelbuettel left a comment

Choose a reason for hiding this comment

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

Excellent stuff -- 🚢

src/mlpack/bindings/R/CMakeLists.txt Show resolved Hide resolved
src/mlpack/bindings/R/mlpack/src/Makevars Show resolved Hide resolved
Copy link

@mlpack-bot mlpack-bot bot left a comment

Choose a reason for hiding this comment

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

Second approval provided automatically after 24 hours. 👍

Copy link
Member

@zoq zoq left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@rcurtin
Copy link
Member Author

rcurtin commented Jun 1, 2023

Yep, it took me a minute to debug the failing test (unrelated to this PR), but now that it's all green I'll go ahead and merge. 👍

@rcurtin rcurtin merged commit 3b893ba into mlpack:master Jun 1, 2023
19 checks passed
@rcurtin rcurtin deleted the install-r-headers branch June 1, 2023 12:38
@eddelbuettel
Copy link
Contributor

Thanks so much for the persistence! Should we make a 4.1.1 release for CRAN?

@rcurtin
Copy link
Member Author

rcurtin commented Jun 1, 2023

I was thinking of waiting on #3487 and other nearly-ready-to-go PRs to be merged, and then releasing the next version of mlpack (thus updating CRAN too). So, within the next week or so?

@eddelbuettel
Copy link
Contributor

That works too and is a wee bit cleaner :)

@conradsnicta
Copy link
Contributor

I was thinking of waiting on #3487 and other nearly-ready-to-go PRs to be merged, and then releasing the next version of mlpack (thus updating CRAN too). So, within the next week or so?

@rcurtin Suggest to label the new release as mlpack 4.2, instead of 4.1.1. The changes are more than just bug fixes -- there are new features. On top of that, people like new shiny things with bigger numbers.

@rcurtin
Copy link
Member Author

rcurtin commented Jun 12, 2023

Agreed, 4.2.0 is just fine given the new features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants