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

Fix up workflow of dart formatting in diplomat-gen #4291

Closed
sffc opened this issue Nov 14, 2023 · 2 comments · Fixed by #4333
Closed

Fix up workflow of dart formatting in diplomat-gen #4291

sffc opened this issue Nov 14, 2023 · 2 comments · Fixed by #4333
Labels
A-ffi Area: FFI, WebAssembly, Transpilation C-ffi-infra Component: Diplomat, horizontal FFI discuss Discuss at a future ICU4X-SC meeting discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band

Comments

@sffc
Copy link
Member

sffc commented Nov 14, 2023

Since Dart FFI landed this month, contributors adding or changing FFI need to have the dart command installed in order to generate clean diffs. The command is only used for formatting.

This is not good because it adds a non-Cargo dependency to a primary ICU4X contributor workflow. (Let's not debate whether or not adding FFI is a primary ICU4X workflow: the fact is that it absolutely is.)

We have a number of approaches:

  1. Keep the current behavior. Contributors need to figure out how to install Dart on their own.
    • Pro: ?
    • Con: Most challenging for contributors
  2. Add cargo make dart-install and make cargo make diplomat-gen fail if dart is not available
    • Pro: Better paved path for contributors
    • Con: Unclear how to make cargo make diplomat-install work on all platforms like Windows, etc.
    • Con: Still requires a non-Cargo dependency
  3. Send an HTTP request to a public service like dartpad.dev for the Dart formatting if dart is not available (printing a notice that such behavior is taking place so that people can install dart to avoid the network call)
    • Pro: Primary contributor workflow just works out of the box
    • Con: Potential that the public service could break or block us at any time
  4. Like 2 but host our own service and use it internally in icu4x tooling
    • Pro: Fewer external variables than using someone else's public service
    • Con: Potential hiccups hosting and maintaining our own service
  5. Don't format the dart code
    • Pro: Most similar to how the other diplomat backends work. As long as the output is "good enough" it should be fine.
    • Pro: Easiest of all these options for all parties involved.
    • Con: Code doesn't look as pretty in the repo and needs to be re-formatted when vendored into a Dart project?
  6. Don't check in the generated dart code
    • Pro: Easy
    • Con: Inconsistent with the other diplomat backends
  7. Remove all generated diplomat code from the repo
    • Pro: Solves this and future similar problems
    • Con: Harder to reference the header files and run reproducible unit tests

Discuss with:

@sffc sffc added A-ffi Area: FFI, WebAssembly, Transpilation discuss Discuss at a future ICU4X-SC meeting C-ffi-infra Component: Diplomat, horizontal FFI discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band labels Nov 14, 2023
@Manishearth
Copy link
Member

Is there a reason our generated code is not already up to scratch formatting wise? Just linewrapping?

My preference would be for Option 4, with some work to make the current generated dart code "close enough" to well-formatted dart code.

cc @mosuem

There's also Option 7: Have a --fmt option for diplomat-tool, and do not typically CI with it, but turn it on right before a release.

@sffc
Copy link
Member Author

sffc commented Nov 21, 2023

@hsivonen reported:

It appears that dart provided by the flutter snap doesn't satisfy the formatting expectations of CI.

robertbastian added a commit that referenced this issue Nov 21, 2023
Removes formatting, fixes #4291
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ffi Area: FFI, WebAssembly, Transpilation C-ffi-infra Component: Diplomat, horizontal FFI discuss Discuss at a future ICU4X-SC meeting discuss-triaged The stakeholders for this issue have been identified and it can be discussed out-of-band
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants