feat: added the build for macos #1246
Conversation
|
🧙 Sourcery has finished reviewing your pull request! Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @Jhalakupadhyay - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
- The path to the Android NDK is hardcoded. (link)
Overall Comments:
- Consider adding a step to notarize the macOS app for distribution outside the Mac App Store.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🔴 Security: 18 blocking issues
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -0,0 +1,803 @@ | |||
| #ifdef __cplusplus | |||
There was a problem hiding this comment.
issue (complexity): Consider centralizing preprocessor definitions into a header file using X-Macros to reduce code duplication and improve maintainability.
Consider isolating this auto‐generated preprocessor clutter into a centralized header (or generated module) to reduce duplication and simplify maintenance. For example, you could move the compiler/platform/architecture detection into one header file and then include it where needed:
// compiler_info.h
#ifndef COMPILER_INFO_H
#define COMPILER_INFO_H
// Use X-Macros to define compiler entries and reduce duplicate conditionals.
#define COMPILER_LIST \
X(__INTEL_COMPILER, "Intel", DEC(__INTEL_COMPILER)) \
X(__clang__, "Clang", DEC(__clang_major__)) \
/* ... add other entries as needed ... */
#define X(compiler_macro, compiler_id, version) \
/* Check for each compiler in a single place */ \
# if defined(compiler_macro) \
# define COMPILER_ID compiler_id \
/* Define compiler version components here */ \
# endif
COMPILER_LIST
#undef X
// Similar centralized sections can be created for PLATFORM_ID and ARCHITECTURE_ID.
#endif // COMPILER_INFO_HActionable Steps:
-
Centralize Definitions: Move the extensive detection code into a separate header (e.g.,
compiler_info.h). -
DRY with X-Macros: Use an X-macro pattern to define compiler entries in one list and avoid repeating similar conditional branches.
-
Include Where Needed: Replace the duplicated preprocessor code in each architecture folder with a simple include of this header.
This preserves functionality while reducing complexity and maintenance burden across architecture-specific folders.
| -DANDROID_PLATFORM=android-21 | ||
| -DANDROID_ABI=$ABI | ||
| -DCMAKE_ANDROID_ARCH_ABI=$ABI | ||
| -DANDROID_NDK=$NDK |
There was a problem hiding this comment.
🚨 issue (security): The path to the Android NDK is hardcoded.
| -DANDROID_ABI=$ABI | ||
| -DCMAKE_ANDROID_ARCH_ABI=$ABI | ||
| -DANDROID_NDK=$NDK | ||
| -DCMAKE_ANDROID_NDK=$NDK |
There was a problem hiding this comment.
🚨 issue (security): The path to the Android NDK is hardcoded.
| -DANDROID_PLATFORM=android-21 | ||
| -DANDROID_ABI=arm64-v8a | ||
| -DCMAKE_ANDROID_ARCH_ABI=arm64-v8a | ||
| -DANDROID_NDK=C:\Users\Jhalak Upadhyay\dev\AndroidSDK\ndk\27.0.12077973 |
There was a problem hiding this comment.
🚨 issue (security): The path to the Android NDK is hardcoded.
| -DANDROID_ABI=arm64-v8a | ||
| -DCMAKE_ANDROID_ARCH_ABI=arm64-v8a | ||
| -DANDROID_NDK=C:\Users\Jhalak Upadhyay\dev\AndroidSDK\ndk\27.0.12077973 | ||
| -DCMAKE_ANDROID_NDK=C:\Users\Jhalak Upadhyay\dev\AndroidSDK\ndk\27.0.12077973 |
There was a problem hiding this comment.
🚨 issue (security): The path to the Android NDK is hardcoded.
| -DCMAKE_ANDROID_ARCH_ABI=x86_64 | ||
| -DANDROID_NDK=C:\Users\Jhalak Upadhyay\dev\AndroidSDK\ndk\27.0.12077973 | ||
| -DCMAKE_ANDROID_NDK=C:\Users\Jhalak Upadhyay\dev\AndroidSDK\ndk\27.0.12077973 | ||
| -DCMAKE_TOOLCHAIN_FILE=C:\Users\Jhalak Upadhyay\dev\AndroidSDK\ndk\27.0.12077973\build\cmake\android.toolchain.cmake |
There was a problem hiding this comment.
🚨 issue (security): The path to the Android NDK is hardcoded.
| set(CMAKE_HOST_SYSTEM_VERSION "10.0.26100") | ||
| set(CMAKE_HOST_SYSTEM_PROCESSOR "AMD64") | ||
|
|
||
| include("C:/Users/Jhalak Upadhyay/dev/AndroidSDK/ndk/27.0.12077973/build/cmake/android.toolchain.cmake") |
There was a problem hiding this comment.
🚨 issue (security): The path to the Android NDK is hardcoded.
| set(CMAKE_HOST_SYSTEM_VERSION "10.0.26100") | ||
| set(CMAKE_HOST_SYSTEM_PROCESSOR "AMD64") | ||
|
|
||
| include("C:/Users/Jhalak Upadhyay/dev/AndroidSDK/ndk/27.0.12077973/build/cmake/android.toolchain.cmake") |
There was a problem hiding this comment.
🚨 issue (security): The path to the Android NDK is hardcoded.
| set(CMAKE_HOST_SYSTEM_VERSION "10.0.26100") | ||
| set(CMAKE_HOST_SYSTEM_PROCESSOR "AMD64") | ||
|
|
||
| include("C:/Users/Jhalak Upadhyay/dev/AndroidSDK/ndk/27.0.12077973/build/cmake/android.toolchain.cmake") |
There was a problem hiding this comment.
🚨 issue (security): The path to the Android NDK is hardcoded.
| set(CMAKE_HOST_SYSTEM_VERSION "10.0.26100") | ||
| set(CMAKE_HOST_SYSTEM_PROCESSOR "AMD64") | ||
|
|
||
| include("C:/Users/Jhalak Upadhyay/dev/AndroidSDK/ndk/27.0.12077973/build/cmake/android.toolchain.cmake") |
There was a problem hiding this comment.
🚨 issue (security): The path to the Android NDK is hardcoded.
Build StatusBuild successful. APKs to test: https://github.com/fossasia/badgemagic-app/actions/runs/13972544467/artifacts/2789199348. Screenshots (Android)
Screenshots (iPhone)
Screenshots (iPad)
|
||||||||||||||||||||||||
-1_home_screen.png?raw=true)
-2_text_badge.png?raw=true){kind=icon}
-3_emoji_badge.png?raw=true){kind=icon}
-4_inverted_emoji_badge.png?raw=true){kind=icon}
-5_saved_badges.png?raw=true)
-6_saved_badges_clicked.png?raw=true)
-7_draw_badge.png?raw=true){kind=icon}
Fixes #1167
Changes
Screenshots / Recordings
Checklist:
constants.dartwithout hard coding any value.Summary by Sourcery
Add a new CI workflow to build the macOS application.
CI: