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

Maintenance PR + Hotfix #154

Merged
merged 20 commits into from
Apr 12, 2023
Merged

Maintenance PR + Hotfix #154

merged 20 commits into from
Apr 12, 2023

Conversation

CGMossa
Copy link
Member

@CGMossa CGMossa commented Apr 10, 2023

UPDATE: This PR got a little out of hand, so here's a description on what it does.

  • get_non_api is only used when use-bindgen is enabled.
  • Rename RCallbacks to reflect what they actually do right now, which is trimming the comments processed by clang, i.e. TrimCommentsCallbacks.
  • BUG FIX: On Windows paths to dylib/dlls don't get processed properly. Added a .canonicalize and it seems to fix it.
  • Updated bindgen to latest version 0.64.
  • Added a versioning header to all bindings; e.g.
/* bindgen clang version: Ubuntu clang version 14.0.0-1ubuntu1 */
/* clang-rs version: Ubuntu clang version 14.0.0-1ubuntu1 */
/* r version: 4.1.3 */
/* automatically generated by rust-bindgen 0.64.0 */
[...]
  • BUG FIX: Added a is_anonymous check on the entities processed. This is necessary as some unamed items
    gets a name in llvm16, and that isn't handled in any meaningful way by clang-rs unfortunately. This requires clang_3_7 atleast, which is also added.
  • Updated bindings

I spent a lot of time inside the build.rs file and found a few things.
See the commit descriptions as well.

  • Ensure generated bindings work with extendr/extendr. (i.e. no missing stuff please!)

CGMossa added 5 commits April 11, 2023 00:28
should only be
present when that happens
for trimming comments
thus they should be named as such
`STATUS_DLL_NOT_FOUND`
error on Windows.
It is something to do with paths.
this rust-lang/cargo#6773
motivated the following change
(I know...)
@CGMossa CGMossa marked this pull request as draft April 10, 2023 22:37
@CGMossa CGMossa changed the title Maintenance PR Maintenance PR + Hotfix Apr 11, 2023
@CGMossa
Copy link
Member Author

CGMossa commented Apr 11, 2023

I've ended up adding a hotfix for the issue that @Ilia-Kosenkov have been hunting down.
We've reported this to rust-lang/rust-bindgen#2488, and then I found out that it could potentially be the addition of unnamed structs in the named list of items in allowlist_pattern.

Needless to say, this should be removed in the future of course, once we get things patched upstream

@CGMossa CGMossa marked this pull request as ready for review April 11, 2023 09:41
@CGMossa
Copy link
Member Author

CGMossa commented Apr 11, 2023

Temporarily fixes #151

@yutannihilation
Copy link
Contributor

Thanks for the great investigation! But, I see suspicious diffs on the bindings. I have no idea why the CI tests won't fail. Any ideas?

@CGMossa
Copy link
Member Author

CGMossa commented Apr 11, 2023 via email

@yutannihilation
Copy link
Contributor

Do you know if the /bindings command invokes CI? Why not?

It doesn't invoke the CI because the commit message contains "[skip ci]".
The intent of this is that, as the bindings are the result of the CI runs, it should have failed if something is wrong.

And, sorry, I thought the CI checks with extendr so it would fail, but I was wrong here.

@CGMossa
Copy link
Member Author

CGMossa commented Apr 11, 2023

While this passes CI, running extendr on this PR doesn't pass. Example is R_BaseEnv is needed, but it got deleted (for some configurtion).

Thus I need to investigate this.. Sorry for the inconvenience.

@CGMossa
Copy link
Member Author

CGMossa commented Apr 11, 2023

Unfortunately, I do not have a path forward. Running this branch locally produces the bindings without the missing elements (atleast the one I sit on).
I can also not find where the logs for generating the bindings is at, so I'm unable to inspect what might have went on on the runner.

I definitely need assistance on this :(

@CGMossa
Copy link
Member Author

CGMossa commented Apr 11, 2023

No matter what I do, I'm unable to replicate the generated file on my machine with the one provided by the CI.

My version and the CI version agree on these

/* bindgen clang version: clang version 16.0.0 */
/* clang-rs version: clang version 16.0.0 */
/* r version: 4.2.3 */
/* automatically generated by rust-bindgen 0.64.0 */

and still there are some items that appear in my generated bindings locally, that are not present in the CI.

@yutannihilation
Copy link
Contributor

Ah, so maybe adding " made the items on the head and tail of the allowlist unmatched?

b59b601

which depends on atleast `clang_3_7`,
so that is added, but hopefully it doesn't
interfer with `runtime`-feature.
@CGMossa
Copy link
Member Author

CGMossa commented Apr 12, 2023

I lost a lot of my day yesterday to this little... issue.
I've updated the PR description and I kindly ask you to review and approve so this can be dealt with for now.

I think what we have right now is as much as an official fix will look like. Not just a hotfix.

@CGMossa
Copy link
Member Author

CGMossa commented Apr 12, 2023

/bindings

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.

Amazing job, really well done! Thanks for also catching some annoying bugs, I'll definitely check it locally, especially whether the search path issue is resolved on Windows.

Feel free to resolve or reject the comments I left, they are mostly stylistic

build.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
@CGMossa
Copy link
Member Author

CGMossa commented Apr 12, 2023

@Ilia-Kosenkov can you confirm that I may merge this?

Copy link
Contributor

@yutannihilation yutannihilation left a comment

Choose a reason for hiding this comment

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

Great work, thanks!

@CGMossa CGMossa merged commit e5ff264 into master Apr 12, 2023
@CGMossa CGMossa deleted the maintenance_build_rs branch April 12, 2023 11:52
@CGMossa CGMossa mentioned this pull request Apr 12, 2023
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.

3 participants