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

Make rextendr packages CRAN compatible by default #394

Merged
merged 13 commits into from
Nov 17, 2024
Merged

Conversation

JosiahParry
Copy link
Contributor

As a followup from #393 and comments from @CGMossa and @Ilia-Kosenkov, this PR makes adds --print=native-static-libs to any existing RUSTFLAGS ensuring that they are not overwritten.

@JosiahParry
Copy link
Contributor Author

Portable Makefiles do not use GNU extensions such as +=, :=,

welp! back to the drawing board!

@Ilia-Kosenkov
Copy link
Member

Can't you do something like RUSTFLAGS = $(RUSTFLAGS) --whatever?
UPD: ChatGPT suggests this (did not test it myself)

RUSTFLAGS := $(RUSTFLAGS) --print=native-static-libs
export RUSTFLAGS

Copy link
Contributor Author

@JosiahParry JosiahParry left a comment

Choose a reason for hiding this comment

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

@Ilia-Kosenkov @CGMossa

I've taken this as an opportunity to refactor the way in which Makevars are done.

Summary

This PR now removes use_cran_defaults() and instead, uses a single Makevars and Makevars.in to support CRAN environment and regular development environment. This is done in the following way:

  • Makevars & Makevars.win are now Makevars.in and Makevars.win.in
  • The configure script generates the Makevars and Makevars.win files substituting the @CRAN_FLAGS@ this approach is taken from data.table see https://github.com/Rdatatable/data.table/blob/master/src/Makevars.in. It also deletes the src/Makevars and src/Makevars.win before generating the respective files
  • Now src/Makevars{.win} are in the .Rbuildignore and .gitignore when using use_extendr() ensuring that the file is always rebuilt
  • When NOT_CRAN=false the vendor.tar.xz is unbundled and the flags -j 2 --offline are set

To ensure all of this is done correctly, two new tests are added which run rcmdcheck::rcmdcheck() and rcmdcheck::rcmdcheck(env = c("NOT_CRAN"="false")) to check both scenarios.

The function use_cran_defaults() has been removed.

Next steps

If you both approve of this direction, I would like to make the following changes

Within this PR

  • remove vignettes/articles/cran-compliance.qmd as an alternative i am happy to write a standalone help doc e.g. ?rextendr::cran
  • remove mentions of use_cran_defaults() from the NEWS.md as the function will have been introduced and removed before a new CRAN release

Follow up PRs

inst/templates/configure Outdated Show resolved Hide resolved
inst/templates/Makevars.in Show resolved Hide resolved
inst/templates/Makevars.in Outdated Show resolved Hide resolved
inst/templates/Makevars.in Show resolved Hide resolved
inst/templates/Makevars.in Outdated Show resolved Hide resolved
inst/templates/Makevars.win.in Outdated Show resolved Hide resolved
tests/testthat/test-cran-compliance.R Outdated Show resolved Hide resolved
tests/testthat/test-use_extendr.R Show resolved Hide resolved
@Ilia-Kosenkov
Copy link
Member

I still believe checking NOT_CRAN for anything other than true is invalid. NOT_CRAN is an optional env var that you can set to, usually, true to signify that you are not on CRAN. In all other scenarios, you treat is as if you are on CRAN. What does NOT_CRAN=false mean? When do you expect to set this?

@JosiahParry
Copy link
Contributor Author

Thanks @Ilia-Kosenkov, that's fine. That can be addressed. But are you okay with this plan (assuming we change the NOT_CRAN to check based on a true value)? Having one ring Makevars to rule them all?

@DavZim
Copy link

DavZim commented Nov 16, 2024

What happens when NOT_CRAN is true but I don't have vendor/ only vendor.tar.xz? Ie I installed the package from GitHub or from local.

Is the archive unpacked correctly?
Btw, once this is done, im happy to upgrade the other blog accordingly.

@JosiahParry
Copy link
Contributor Author

@DavZim the Makevars is only ran when building the package from source. This template will unpack the archive if NOT_CRAN is unset and the vendor.tar.xz file exists. If NOT_CRAN=true the case when using devtools etc then the archive is not unpacked.

@JosiahParry JosiahParry changed the title Make RUSTFLAGS modification additive Make rextendr packages CRAN compatible by default Nov 17, 2024
@JosiahParry JosiahParry marked this pull request as draft November 17, 2024 17:47
@JosiahParry
Copy link
Contributor Author

I've moved this as a draft so I can adjust the configure scripts to set CRAN_FLAGS when NOT_CRAN is unset and a vendor.tar.xz folder is present.

@JosiahParry JosiahParry marked this pull request as ready for review November 17, 2024 18:22
@JosiahParry JosiahParry enabled auto-merge (squash) November 17, 2024 18:27
@JosiahParry JosiahParry merged commit 4aefce1 into main Nov 17, 2024
35 checks passed
@JosiahParry JosiahParry deleted the additive-rustflags branch November 17, 2024 18:38
Copy link

codecov bot commented Nov 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.12%. Comparing base (d58284b) to head (d2db22e).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

Files with missing lines Coverage Δ
R/cran-compliance.R 78.37% <ø> (+2.72%) ⬆️
R/use_extendr.R 98.98% <100.00%> (+0.05%) ⬆️

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.

4 participants