Skip to content
This repository was archived by the owner on Jan 28, 2024. It is now read-only.

Conversation

@dcharkes
Copy link
Contributor

@dcharkes dcharkes commented Aug 2, 2022

  # Sanity check the latest `flutter create --template plugin_ffi`.
  # This will break if we change the Flutter template or the generated code.
  # But, getting libclang on the LUCI infrastructure has proven to be
  # non-trivial. See discussion on
  # https://github.com/flutter/flutter/issues/105513.
  # If we need to change the generated code, we should temporarily disable this
  # test, or temporarily disable the requirement for all bots to be green to
  # merge PRs.
  # Running this sanity check on one OS should be sufficient. Chosing Windows
  # because it is the most likely to break.

Integration test for:

@dcharkes
Copy link
Contributor Author

dcharkes commented Aug 2, 2022

The test currently fails because

is not in Flutter yet.

dcharkes added a commit to flutter/flutter that referenced this pull request Aug 2, 2022
Closes:

* #105513

Will add a regression test on the FFIgen repo after merge:

* dart-archive/ffigen#433
@dcharkes dcharkes requested a review from liamappelbe August 2, 2022 08:44
@dcharkes
Copy link
Contributor Author

dcharkes commented Aug 5, 2022

@liamappelbe PTAL

Copy link
Contributor

@liamappelbe liamappelbe left a comment

Choose a reason for hiding this comment

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

Approved with comments.

final bindingsGeneratedCopyUri =
libDirUri.resolve('${projectName}_bindings_generated_copy.dart');

await Task.serial([
Copy link
Contributor

Choose a reason for hiding this comment

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

All this Task stuff seems like a lot of boilerplate. Why not just write a runProcess and copyFile function, and do:

await runProcess(...);
await copyFile(...);
await runProcess(...);

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 had this laying around in another project. I can simplify it for this test if you prefer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to merge as-is if you're too busy to clean it up. It's only a test after all. I've already approved.

@dcharkes dcharkes merged commit 08756db into master Aug 15, 2022
@dcharkes dcharkes deleted the flutter-ci branch August 15, 2022 17:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants