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.

Sign up for GitHub

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

Jump to bottom

Use allowlist to keep the exports minimal #96

Merged
merged 8 commits into from
Jan 5, 2023
Merged

Use allowlist to keep the exports minimal #96

merged 8 commits into from
Jan 5, 2023

Conversation

yutannihilation
Copy link
Contributor

@yutannihilation yutannihilation commented Feb 12, 2022 •
edited
Loading

See #95 (comment)

Currently, libR-sys exports all symbols wildly, including those from the C standard library. I think we should keep the exports minimal to avoid confusion. This pull request tries to create a proper allowlist. The basic idea is to extract and parse the include files by ourselves first and construct an allowlist because there seems not measure to stop bindgen to parse the C standard library.

@yutannihilation
Squashed commit of the following:
51bfb48 
commit e2b33b5
Author: Hiroaki Yutani <[email protected]>
Date:   Sat Feb 12 19:49:20 2022 +0900

    Further tweak

commit 5f89940
Author: Hiroaki Yutani <[email protected]>
Date:   Sat Feb 12 19:25:58 2022 +0900

    [PoC] Use allowlist to keep the exports minimal

commit 9ff593b
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 10 23:30:38 2022 +0900

    Fix bindings name

commit 45dc8c6
Author: Hiroaki Yutani <[email protected]>
Date:   Fri Feb 4 20:50:53 2022 +0900

    Do not expose DEP_R_R_VERSION_STRING

commit bb2ad61
Author: Hiroaki Yutani <[email protected]>
Date:   Fri Feb 4 20:37:33 2022 +0900

    Improve a comment

commit 96137e0
Author: Hiroaki Yutani <[email protected]>
Date:   Fri Feb 4 20:36:47 2022 +0900

    Improve a comment

commit bc128e0
Author: Hiroaki Yutani <[email protected]>
Date:   Fri Feb 4 20:36:40 2022 +0900

    Implement get_r_binary()

commit fbb5488
Author: Hiroaki Yutani <[email protected]>
Date:   Fri Feb 4 20:23:13 2022 +0900

    Fix r_version_devel

commit c13d779
Author: Hiroaki Yutani <[email protected]>
Date:   Fri Feb 4 20:22:06 2022 +0900

    Rename LIBR_SYS_R_VERSION to LIBRSYS_R_VERSION

commit 8f27931
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 09:47:45 2022 +0900

    Make environmental variables const

commit 84b2f42
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 09:16:48 2022 +0900

    Fix clippy warning or_fun_call

commit c477bb1
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 09:13:17 2022 +0900

    Improve comment

commit 05a6f49
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 09:08:21 2022 +0900

    Reject empty version string

commit 6f66e41
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 08:58:36 2022 +0900

    Retry only when the envvar is not set

commit c13c80f
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 08:50:07 2022 +0900

    Fix unused import

commit e8d1df4
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 08:43:08 2022 +0900

    Fix a typo

commit 36d42e9
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 02:32:22 2022 +0900

    Fix comment

commit 98c5574
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 02:19:52 2022 +0900

    Rustfmt

commit 1962625
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 02:19:45 2022 +0900

    Refactor parseing R version

commit f85d498
Author: Hiroaki Yutani <[email protected]>
Date:   Thu Feb 3 00:03:46 2022 +0900

    Refactor get_r_version_from_r

commit 70fe57f
Author: Hiroaki Yutani <[email protected]>
Date:   Wed Feb 2 23:13:57 2022 +0900

    Refactor probe_r_paths()
This was referenced Feb 17, 2022
Closed
Closed
@yutannihilation yutannihilation changed the title [PoC] Use allowlist to keep the exports minimal Use allowlist to keep the exports minimal Dec 20, 2022
@yutannihilation yutannihilation marked this pull request as ready for review December 20, 2022 03:52
@yutannihilation
Copy link
Contributor Author

@Ilia-Kosenkov @andy-thomason
This isn't very high priority and probably is difficult to understand, but I'd like to hear your thoughts on this. I think this is a necessary move not only to prevent such confusions as #95, but also to reduce the code size. Note that I've been using this branch for my CRAN package without any problem, so I believe this is ready to merge basically.

@yutannihilation
Copy link
Contributor Author

@Ilia-Kosenkov
May I merge this?

@Ilia-Kosenkov
Copy link
Member

Looks good, but let me restart CI since the last run was ~2 weeks ago -- to be on the safe side.

Ilia-Kosenkov
Ilia-Kosenkov approved these changes Jan 5, 2023
View reviewed changes
Copy link
Member

@Ilia-Kosenkov Ilia-Kosenkov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks & sorry for delay

@yutannihilation
Copy link
Contributor Author

No worries. Thanks for reviewing!

@yutannihilation yutannihilation merged commit 4bf41e9 into extendr:master Jan 5, 2023
@yutannihilation yutannihilation deleted the poc/minimalish-exports branch January 5, 2023 17:01
@yutannihilation yutannihilation mentioned this pull request Jan 6, 2023
Merged
CGMossa pushed a commit to CGMossa/libR-sys that referenced this pull request Jan 21, 2024
@yutannihilation @CGMossa
Use allowlist to keep the exports minimal (extendr#96)
286b615
CGMossa pushed a commit to CGMossa/libR-sys that referenced this pull request Jan 21, 2024
@yutannihilation @CGMossa
Use allowlist to keep the exports minimal (extendr#96)
356d2d4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ilia-Kosenkov Ilia-Kosenkov Ilia-Kosenkov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yutannihilation @Ilia-Kosenkov