Skip to content

[urgent] Update non-API for CRAN #809

Merged
CGMossa merged 11 commits intomasterfrom
remove_nonapi_june2024
Jun 28, 2024
Merged

[urgent] Update non-API for CRAN #809
CGMossa merged 11 commits intomasterfrom
remove_nonapi_june2024

Conversation

@CGMossa
Copy link
Member

@CGMossa CGMossa commented Jun 25, 2024

CRAN compliance PR.

Background: CRAN has new requirements for what is non-API. These requirements just came up and became defacto law of the land. They are not tied to any R release. We just have to deal with it.

We pulled non-API list in
extendr/libR-sys#248

This PR syncs libR-sys with this commit.

  • Hid previous available functionality behind non-api feature. It means, if you want it, you may compile extendr-api with feature = ["non-api"]. This requires you to build your own bindings by way of libR-sys as wel.
  • Moved all doc-tests to non_api_tests, and in order to make that work, I had to add an entry for it in extendr-api/Cargo.toml. Note that the optional-module in integration tests doesn't work. And it needs to also have an entry to work.
  • I also added required-features, but this doesn't run the test unless the feature is provided to the cargo test-call. So these tests are present, but won't run. We have to have a broader conversation about testing non-api. For now, this is a patch, that allows us to rediscover missing features if users want them.

dfalbel added a commit to mlverse/tok that referenced this pull request Jun 25, 2024
@JosiahParry
Copy link
Contributor

Overall, I am appreciative of these changes.

Though I wonder if there is utility in keeping the removed doc tests and instead of having them wrap test! they can be standard markdown blocks for standard documentation purposes.

While we should entirely revamp that page, I don't want to lose the examples that are there.

Can we either:

  1. remove the test! macro or
  2. Comment these out instead of removing entirely?

@CGMossa
Copy link
Member Author

CGMossa commented Jun 25, 2024

Oh. I don't know.

Didn't think of either of those options to be honest.
For (2) I'd say some people hate commented-out code in a code-base, and for (1) if they don't actually work, why should they still be present?

The removed doc-test is because these invocations won't work anymore.

EIther way, I welcome changes to this PR. I'm not planning on doing more here.

Copy link
Contributor

@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.

I have one question regarding scope of the mutable assignments.

But we have a lot to add to the changelog. And should add receipts from r-devel exchanges as well.

@yutannihilation
Copy link
Contributor

  • Hid previous available functionality behind non-api feature.

While I agree this is a good idea for a temporary solution, I'd recommend you to re-implement the functionality using the provided APIs (or simply remove them). I think the R core team is cleaning up these "non-API"s, so it's possible there's no time until they actually remove them (I guess not all, but some), which means extendr won't compile with the non-api feature.

dfalbel added a commit to mlverse/tok that referenced this pull request Jun 26, 2024
* Update to extendr-api v0.6.0

* Go offline

* Downgrade regex for building on older rust toolchain.

* updates to use Extendr from main

* buildignore the .vscode dir

* Change the error type

* Workaround bug in dev extendr

* add a r-devel check

* Upgrade minimum Rust toolchain required to 1.69

* Bundle extendr from extendr/extendr#809

* re-vendor for line endings

* Try cleaning up header right after building.

* Instead simply cleanup all headers
Copy link
Member Author

@CGMossa CGMossa left a comment

Choose a reason for hiding this comment

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

The CHANGELOG works for me.

@JosiahParry
Copy link
Contributor

JosiahParry commented Jun 28, 2024

Ubuntu CI is failing due to multithreading of the cargo tests which is poisoning the thread safety mutex. This can be fixed by #674

The windows tests are failing at extendr-api/tests/extendr_macro.rs

@CGMossa
Copy link
Member Author

CGMossa commented Jun 28, 2024

If I rebase, then the meta data tests will succeed.

@CGMossa CGMossa force-pushed the remove_nonapi_june2024 branch from 8ba040d to 0fc50c8 Compare June 28, 2024 13:55
@CGMossa
Copy link
Member Author

CGMossa commented Jun 28, 2024

I have one question regarding scope of the mutable assignments.

But we have a lot to add to the changelog. And should add receipts from r-devel exchanges as well.

I've now rebased. I don't understand what to reply to this. Is it possible to do or not?

@CGMossa CGMossa merged commit 5c7cbe8 into master Jun 28, 2024
@CGMossa CGMossa deleted the remove_nonapi_june2024 branch June 28, 2024 14:35
CGMossa added a commit that referenced this pull request Oct 28, 2024
* Cargo.toml: update libR-sys to latest commit

* added more `non-api` cfg guards

* moved doctests to their own module

* wp: another

* wp: another one

* this test is disabled, yet it still compiles..

* typo

* removed more tests

* update changelog

* update note on CRAN

* `cargo extendr fmt` [skip ci]

---------

Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
eitsupi pushed a commit to rpolars/extendr that referenced this pull request Mar 13, 2025
* Cargo.toml: update libR-sys to latest commit

* added more `non-api` cfg guards

* moved doctests to their own module

* wp: another

* wp: another one

* this test is disabled, yet it still compiles..

* typo

* removed more tests

* update changelog

* update note on CRAN

* `cargo extendr fmt` [skip ci]

---------

Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
eitsupi pushed a commit to rpolars/extendr that referenced this pull request Mar 13, 2025
* Cargo.toml: update libR-sys to latest commit

* added more `non-api` cfg guards

* moved doctests to their own module

* wp: another

* wp: another one

* this test is disabled, yet it still compiles..

* typo

* removed more tests

* update changelog

* update note on CRAN

* `cargo extendr fmt` [skip ci]

---------

Co-authored-by: Josiah Parry <josiah.parry@gmail.com>
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