Skip to content

Enhance README and CMakeLists for OpenCV module customization #355

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

abdelaziz-mahdy
Copy link
Contributor

@abdelaziz-mahdy abdelaziz-mahdy commented May 8, 2025

should fix #313, didnt try it yet though, but the yaml option was annyoing to implement and found out this option will make it easier to use, if it looks good on your side we can add an example to test it i guess

This pull request introduces a new feature to allow customization of OpenCV modules in the opencv_dart and opencv_core packages. It adds support for a user-defined configuration file (opencv_config.cmake) to enable or disable specific OpenCV modules during the build process. The changes include updates to documentation and build scripts to support this feature.

Edit by rainyl:
TODO:

  • dart parser script
  • opencv_core ?
    • CMakeLists.txt
    • readme
    • example
  • opencv_dart
    • CMakeLists.txt
    • readme
    • example

- Added documentation on customizing OpenCV modules via `opencv_config.cmake` in both `opencv_core` and `opencv_dart` READMEs.
- Updated `CMakeLists.txt` in both packages to include user-defined configurations if `opencv_config.cmake` exists, falling back to default settings otherwise.
@codecov-commenter
Copy link

codecov-commenter commented May 8, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.26%. Comparing base (57bd2c3) to head (82bee34).
Report is 1 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #355   +/-   ##
=======================================
  Coverage   93.26%   93.26%           
=======================================
  Files          50       50           
  Lines       10266    10266           
=======================================
  Hits         9575     9575           
  Misses        691      691           
Flag Coverage Δ
unittests 93.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@abdelaziz-mahdy
Copy link
Contributor Author

abdelaziz-mahdy commented May 8, 2025

also i dont know how to handle that in case of macos and ios :)

i guess even that will be fixed by native assets

@rainyl
Copy link
Owner

rainyl commented May 9, 2025

should fix #313, didnt try it yet though, but the yaml option was annyoing to implement and found out this option will make it easier to use, if it looks good on your side we can add an example to test it i guess

This pull request introduces a new feature to allow customization of OpenCV modules in the opencv_dart and opencv_core packages. It adds support for a user-defined configuration file (opencv_config.cmake) to enable or disable specific OpenCV modules during the build process. The changes include updates to documentation and build scripts to support this feature.

@abdelaziz-mahdy the solution is doable but also dangerous, include() means users can do anything including deletion and malicious code injection, although they do have full control of the package source code, but this will make it easier to attack.

Actually even though #313 is opened, no many demands received for now, and just like you mentioned, it will be solved by native-assets, therefore I didn't active to solve it.

also i dont know how to handle that in case of macos and ios :)

This will be easier, we can only provide 'DartCvIOS/core' in opencv_core.podspec and require users to explicitly provide needed modules inside the podfile.

# <project>/ios/Podfile

target 'Runner' do
  use_frameworks!
  use_modular_headers!

  flutter_install_all_ios_pods File.dirname(File.realpath(__FILE__))
  target 'RunnerTests' do
    inherit! :search_paths
  end
  pod 'DartCvIOS/imgproc' '4.11.0.2' # add this
end

# ...

So in summary, if we want to support the optional modules from now, I would also prefer parsing the specific options in pubspec.yaml following the format like (same as dart user-defines dart-lang/native#39)

hooks:
  user_defines:
    dartcv4:
      exclude_modules:
        - contrib
        - dnn
        - features2d

which means we need to parse yaml or at least filter the needed options in CMakelists.txt.

@abdelaziz-mahdy
Copy link
Contributor Author

Oh actually I really didn't consider the malicious intents in my implementation

Yaml will require parsing and generation of cmake options based on it

Will check if I am able to do it or not, but I didn't understand the iOS/macos parts

@rainyl
Copy link
Owner

rainyl commented May 9, 2025

Yaml will require parsing and generation of cmake options based on it

Yes, you can try to ask GPT or claude from some help.

but I didn't understand the iOS/macos parts

currently, the full dependency of DartCvIOS is specified inside opencv_core.podspec, but I have split the modules into several parts in DartCvIOS, which means, users can just add some modules to their Podfile to introduce them, this can be done inside the <flutter_project>/ios/Podfile like I mentioned before to avoid unnecessary modules, of course, in this way, we need to change the opencv_core.podspec to replace the full dependency of DartCvIOS with DartCvIOS/core (core module is always needed), similar to macos.

- Updated README files for both `opencv_core` and `opencv_dart` to reflect the new method of customizing OpenCV modules via the `pubspec.yaml` file instead of `opencv_config.cmake`.
- Introduced a Dart script to generate `dartcv_modules.cmake` based on the specified modules in `pubspec.yaml`.
- Modified `CMakeLists.txt` in both packages to include the generated `dartcv_modules.cmake`, ensuring proper module configuration during the build process.
@abdelaziz-mahdy
Copy link
Contributor Author

I played around with ai/cursor to create the yaml parser and integrate it, so let me know if it makes sense now or not, and i cant see the logs when building with cmake, so i really dont know if its working or not (also i think android examples needs to be updated for gradle)

@rainyl
Copy link
Owner

rainyl commented May 9, 2025

I played around with ai/cursor to create the yaml parser and integrate it, so let me know if it makes sense now or not, and i cant see the logs when building with cmake, so i really dont know if its working or not

looks good, will test it. you can pass -v option when running flutter build and the detailed logs will be shown, also you can use flutter build apk -v > build.log to redirect them to a file if it's too long. :)

also i think android examples needs to be updated for gradle

Feel free to do it.

abdelaziz-mahdy and others added 4 commits May 9, 2025 23:22
- Upgraded Android Gradle plugin from version 8.1.0 to 8.10.0.
- Updated Gradle wrapper distribution from version 8.5 to 8.11.1.
@rainyl
Copy link
Owner

rainyl commented May 10, 2025

I just got some time and tested it, finding that your current implementation is okay but not elegant enough, e.g., cmake can't get user's project directory and flutter doesn't provide a concrete way to get it, and it will be better if the generation step doesn't produce any files.

Therefore, I have made some changes for dartcv4 and opencv_core, basically add a yaml parser inside packages/dartcv/bin, making it output the constructed string and calling the command inside CmakeLists.txt like you did, and parsing the output string using Regexp and set needed variables, in this way, no output files needed and will be safer and cleaner.

The current implementation works on android but need to be tested on linux and windows, you can try to do it if you have some time, if not I will do it, maybe in the next week if I have some free time, too busy recently :(

@abdelaziz-mahdy
Copy link
Contributor Author

Thank you for letting me know the changes, yeah I thought about dealing with the paths and file generated will be annoying and as you mentioned before maybe someone will inject commands into it

This is why I went with the string approach, and I don't have Linux nor windows machines so I can't test it sadly 🥹

Also thank you and take your time I don't want the feature currently but I liked the addition and thought it will be great for users experience

@rainyl
Copy link
Owner

rainyl commented May 10, 2025

I have edited the PR to add a todo list, hope you won't mind it.

Also, if the optional modules supported, I am considering whether it's necessary to keep the package opencv_core, since the only difference between opencv_core and opencv_dart is the supported modules, maybe we can deprecate opencv_core?

@rainyl
Copy link
Owner

rainyl commented May 10, 2025

This is why I went with the string approach, and I don't have Linux nor windows machines so I can't test it sadly 🥹

Aha, it's okay, I will test it sometime.

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.

Provide a way to disable modules to reduce package size
3 participants