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

Exclude non-API calls #135

Merged

Conversation

yutannihilation
Copy link
Contributor

@yutannihilation yutannihilation commented Feb 25, 2023

Close #132

The main change of this pull request is preventing the "non-API" calls from being included in the allowlist. These "non-API" are APIs that are available but prohibited to use. What does "prohibited" mean? It means R CMD check warns, so a package using extendr won't be accepted by CRAN. So, it's better not to expose them.

To be clear, (except that there's no guarantee that these APIs can get changed without any announcement) it's not a problem as long as the user doesn't have the ambition to submit their R package to CRAN. Actually, the extendr-engine crate some of these. So we have to keep them exported.

The list of "non-API" symbols can be found in tools:::nonAPI. To avoid complexity, this is dumped to a text file nonAPI.txt outside of build.rs, and periodically updated by the GitHub Actions CI; when there's any change on the list, the CI automatically creates a pull request like this: yutannihilation#19.

@yutannihilation
Copy link
Contributor Author

/bindings

@yutannihilation
Copy link
Contributor Author

I updated the branch using my forked repo: yutannihilation#15

@yutannihilation
Copy link
Contributor Author

Hmm, strange. free_R_HOME and freeRUser were added 3 weeks ago. Is macOS's R-devel this outdated?

https://github.com/wch/r-source/blame/8c7dcc5779eb9bdebd9e26a4a0dbd26169c13a6c/src/library/tools/R/sotools.R#L462

@yutannihilation
Copy link
Contributor Author

Is macOS's R-devel this outdated?

Oh, Intel mac seems to keep failing...

image

https://mac.r-project.org/

Anyway, I realize checking these list eagerly is unstable and not a good idea. I'll move it to a separate action just for updating nonAPI.txt.

@yutannihilation
Copy link
Contributor Author

/bindings

@yutannihilation
Copy link
Contributor Author

Okay, sorry for being noisy. This should be ready now, but I don't want to merge this before publishing v0.4.0. Adding APIs should be safe, but removing might not.

I'll write the summary in description, but in short,

  • The main change of this pull request is preventing the "non-API" calls from being included in the allowlist.
  • The list of "non-API" symbols can be found in tools:::nonAPI. To avoid complexity, this is dumped to a text file nonAPI.txt and periodically updated by the CI.

@yutannihilation yutannihilation marked this pull request as ready for review February 26, 2023 13:14
Copy link
Member

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

If you have time to tell us why these things should be hidden, that would be great. But sure, non-api things that are exposed due to C being C, is understandable.

@yutannihilation
Copy link
Contributor Author

Thanks.

why these things should be hidden

To prevent tragedy like extendr/extendr#424

@yutannihilation
Copy link
Contributor Author

/bindings

@yutannihilation yutannihilation merged commit 9250c5c into extendr:master Mar 1, 2023
@yutannihilation yutannihilation deleted the refactor/remove-non-API-calls branch March 1, 2023 16:26
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.

Exclude "non-API" defined in R's source code
2 participants