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

FFIgenPad #1390

Draft
wants to merge 123 commits into
base: main
Choose a base branch
from
Draft

FFIgenPad #1390

wants to merge 123 commits into from

Conversation

TheComputerM
Copy link
Contributor

This is a PR for adding FFIgenPad (a GSoC proposal)

  • Bulk of the code is directly lifted from ffigen 14-wip, some major changes are
    • @Native bindings instead of DynamicLibrary cause of dart2wasm
    • use of a wrapper.c (used to make functions that play well with pointers) cause dart2wasm currently doesn't play well with structs
    • header_parser/clang_bindings/clang_types.dart and header_parser/clang_bindings/clang_wrapper.dart aim to provide 1:1 parity with clang_types and the Clang class in ffigen respectively, this leads to the only changes in many files being the import paths
  • parts which are constant and independent, (like most of lib/src/strings.dart) are exported directly from ffigen
  • The website is located in the web directory, currently uses nodejs.

FFIgenPad also requires a wasm build of libclang, bundled with wrapper.c, which currently is being tracked with git but there are no instructions on how to setup and build it, I'll probably add it soon though.


  • I’ve reviewed the contributor guide and applied the relevant portions to this PR.
Contribution guidelines:

Note that many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.

@github-actions github-actions bot added the type-infra A repository infrastructure change or enhancement label Aug 19, 2024
@TheComputerM TheComputerM marked this pull request as ready for review August 21, 2024 10:09
@TheComputerM
Copy link
Contributor Author

A demo of the web app: https://ffigenpad.surge.sh/

@TheComputerM
Copy link
Contributor Author

@dcharkes @eyebrowsoffire I think we are ready of review.

@Levi-Lesches
Copy link
Contributor

Hey, I've been subscribed to the PR for a while now, nice work! I noticed there's a complete copy of ffigen in the code, and I was wondering if maybe it can be imported instead so it stays up-to-date with the version on pub.dev?

@TheComputerM
Copy link
Contributor Author

TheComputerM commented Aug 21, 2024

@Levi-Lesches thanks for your interest! there are still multiple 'issues' preventing me from doing that, I have listed them on this blog post, in short stuff like loading dylibs and ffi.Structs are not really patchable if you directly import from ffigen.

I am also experimenting with a way we can use patch changes to somehow upstream changes.

Copy link

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

A lot of these files are copy-pasta'd from the ffigen library (with some modifications). Could you add a comment to the top of each of those that indicates which file it is derived from?

@TheComputerM
Copy link
Contributor Author

A lot of these files are copy-pasta'd from the ffigen library (with some modifications). Could you add a comment to the top of each of those that indicates which file it is derived from?

@eyebrowsoffire I made sure to keep the project structure the same (ie ffigen/lib/src/code_generator/type.dart is just at ffigenpad/lib/src/code_generator/type.dart), is there really a need for a separate comment? (it may also mess with the git patch thingy)

@eyebrowsoffire
Copy link

A lot of these files are copy-pasta'd from the ffigen library (with some modifications). Could you add a comment to the top of each of those that indicates which file it is derived from?

@eyebrowsoffire I made sure to keep the project structure the same (ie ffigen/lib/src/code_generator/type.dart is just at ffigenpad/lib/src/code_generator/type.dart), is there really a need for a separate comment? (it may also mess with the git patch thingy)

How about a section of the readme that lists which subsets of the tree are derived from other sources and where to find them? I know you have kept the project structure the same, but even just reviewing this code right now it is not clear to me what is a new file and what is a copied/modified file.

Copy link
Collaborator

@dcharkes dcharkes left a comment

Choose a reason for hiding this comment

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

Sorry for the delay!


- run: dart pub get
working-directory: pkgs/ffigenpad

Copy link
Collaborator

Choose a reason for hiding this comment

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

Run dart analyze and dart format on the code on the CI here.

pkgs/ffigenpad/CHANGELOG.md Outdated Show resolved Hide resolved
pkgs/ffigenpad/analysis_options.yaml Outdated Show resolved Hide resolved
}
}

// TODO: look into better color formatting
Copy link
Collaborator

Choose a reason for hiding this comment

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

File bugs on dart-lang/native and put the URL in TODO(<url>): <short decription>.

// Log all headers for user.
_logger.info('Input Headers: ${config.entryPoints}');

// TODO: for some reason it can't add to a List of pointers
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto


### include/

Contains header files for libclang that are used by ffigen to generate dart:ffi bindings.
Copy link
Collaborator

Choose a reason for hiding this comment

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

dart:ffi in backticks.

Is there a script to update include/clang-c/ from the clang build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's needed but I'll add it.

pkgs/ffigenpad/README.md Outdated Show resolved Hide resolved
pkgs/ffigenpad/README.md Show resolved Hide resolved
pkgs/ffigenpad/README.md Show resolved Hide resolved
cd web
npm i # install dependencies
npm run build
# preview with: npm run preview
Copy link
Collaborator

Choose a reason for hiding this comment

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

@TheComputerM TheComputerM marked this pull request as draft September 5, 2024 05:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-infra A repository infrastructure change or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants