Main

[navedjuwale-creator]

No description provided.

[navedjuwale-creator]

Thank You

[navedjuwale-creator]

Review for Pull Request #17: Add build scripts for Sui Unity SDK supporting multiple platforms

Hi @navedjuwale-creator! Thank you for contributing build automation scripts for the Sui Unity SDK. Supporting multiple platforms is a great step toward making the SDK more developer-friendly. Below are my observations and suggestions based on the files you've added.

---

Overall Impressions

The PR adds:

· Assets/Editor/BuildScript.cs – likely a Unity Editor script for building.
· build.bat and build.sh – platform-specific batch/shell scripts.
· BUILD_SYSTEM.md – documentation for the build system.

This structure is clean and separates the Unity Editor logic from command-line entry points. Documentation is always appreciated. However, I have a few questions and recommendations to ensure the scripts are robust and maintainable.

---

Specific File Reviews

1. Assets/Editor/BuildScript.cs
   (Note: I only have a partial view of this file from the commit history; a full review would require seeing the entire code.)

· Purpose: It likely contains methods for building the SDK for different platforms (e.g., standalone, mobile, WebGL).
· Suggestions:
· Use BuildPipeline.BuildPlayer with appropriate BuildTarget and BuildOptions.
· Consider making platform-specific build settings configurable via ScriptableObjects or command-line arguments.
· Add error handling and logging (e.g., Debug.LogError, EditorUtility.DisplayDialog).
· If the script is intended for CI/CD, ensure it can be called from the command line with -executeMethod.
· Check for platform-specific preprocessor directives (e.g., #if UNITY_IOS) if any code differs per platform.
· Verify that all required scenes are included in the build.

2. build.bat and build.sh

· These are likely wrappers to invoke Unity in batch mode with the appropriate method.
· Checklist:
· Do they set the correct Unity path? Hardcoding may break on different machines; consider using environment variables or relative paths.
· Are they passing necessary arguments (e.g., -projectPath, -buildTarget, -logFile)?
· Do they handle exit codes to indicate success/failure in CI?
· Are both scripts tested on Windows and Unix-like environments?
· It might be helpful to add a build.ps1 for PowerShell users.

3. BUILD_SYSTEM.md

· This is a great addition! Please ensure it covers:
· Prerequisites (Unity version, SDK setup).
· How to run the build scripts (command-line examples).
· How to customize builds (e.g., changing target platform, output path).
· Troubleshooting common issues.
· If the build script supports CI/CD integration, mention that.

---

General Suggestions

· Modularity: Consider splitting BuildScript.cs into smaller files (e.g., AndroidBuild.cs, iOSBuild.cs) if the logic becomes complex.
· Configuration: Allow users to specify build settings (e.g., development build, scripting backend) via command-line arguments or a config file.
· Testing: Mention in the documentation how to test the built SDK (e.g., importing into a sample Unity project).
· Cross-platform consistency: Ensure that the same build steps produce consistent results across Windows, macOS, and Linux (if supported).

---

Questions for the Author

1. Which platforms are supported by these scripts? (e.g., Android, iOS, WebGL, Windows, macOS)
2. Have you tested the builds on all intended platforms? If so, what were the results?
3. Does the build script automatically handle platform-specific dependencies (like the Sui Rust SDK for mobile)?
4. Is there any way to override default build parameters without modifying the script?
5. Could you add a simple example of calling the build method from the command line in the BUILD_SYSTEM.md?

---

Next Steps

I recommend addressing the points above, especially ensuring the scripts are well-documented and flexible. Once these are clarified/updated, I’d be happy to take another look. This is a solid foundation for automating Sui Unity SDK builds! 🚀

---

Note: Since I don’t have the full content of BuildScript.cs, my feedback is somewhat general. If you can share the complete code, I can provide more specific comments.

[navedjuwale-creator]

Thank you for submitting this PR to add build scripts for the Sui Unity SDK with multi-platform support. This is a valuable addition that will help developers automate their builds and streamline integration. I’ve taken a look at the changes and have some feedback and questions to help refine the implementation.

---

Overall Impressions

The PR includes:

· Assets/Editor/BuildScript.cs – likely the Unity Editor build logic.
· build.bat and build.sh – command-line wrappers for different OS environments.
· BUILD_SYSTEM.md – documentation, which is always appreciated!

The structure is clean, and separating the Editor script from platform-specific wrappers is a good approach. Below are some suggestions to enhance robustness and usability.

---

Specific Feedback

1. Assets/Editor/BuildScript.cs

· Functionality: It’s great that you’ve placed this in an Editor folder to avoid runtime inclusion.
· Suggestions:
· Consider using BuildPipeline.BuildPlayer with explicit BuildTarget and BuildOptions.
· Add error handling (try-catch) and log build results clearly (e.g., Debug.LogError or EditorUtility.DisplayDialog).
· If this is intended for CI/CD, ensure the build method can be invoked via -executeMethod and that it returns appropriate exit codes.
· Could you make platform-specific settings (like output paths, scene lists) configurable via command-line arguments or a config file? This would increase flexibility without requiring script edits.
· If there are any platform-specific preprocessor directives (e.g., #if UNITY_IOS), please verify they are correctly applied.

2. build.bat and build.sh

· Purpose: These wrappers are helpful for developers who want to trigger builds from the command line.
· Checklist:
· Do they locate the Unity executable correctly? Hardcoded paths might break on different setups. Consider using environment variables (like UNITY_HOME) or allowing the user to pass the path as an argument.
· Are all necessary command-line arguments passed (e.g., -projectPath, -buildTarget, -logFile)?
· Do the scripts handle exit codes to indicate build success/failure? This is crucial for CI integration.
· Have you tested both scripts on their respective platforms (Windows for .bat, Linux/macOS for .sh)?
· A build.ps1 for PowerShell users might also be a nice addition for cross-platform consistency.

3. BUILD_SYSTEM.md

· Documentation: Thank you for including this! To make it even more helpful, please consider adding:
· Prerequisites (Unity version, SDK dependencies, etc.).
· Step-by-step instructions for running a build (e.g., ./build.sh --target Android).
· How to customize builds (changing output directory, enabling development builds, etc.).
· Troubleshooting tips for common issues (e.g., Unity license activation in CI).
· An example of integrating these scripts into a CI pipeline (GitHub Actions, GitLab CI, etc.) would be a huge plus.

---

Questions for You

1. Which platforms are currently supported by the build scripts (e.g., Android, iOS, WebGL, Windows, macOS)?
2. Have you tested the builds on all intended platforms? If so, what were the results?
3. Does the script automatically handle platform-specific dependencies (like the Sui Rust SDK for mobile)? If not, how should users ensure those are present?
4. Is there a way to override build parameters (like scene list or output path) without modifying the script?
5. Could you provide a simple command-line example in the documentation (e.g., Unity -batchmode -executeMethod Sui.BuildScript.PerformBuild -buildTarget Android)?

---

Next Steps

Please address the points above and update the PR accordingly. Once you’ve made the changes, I’d be happy to review again. This is a solid foundation, and with a few tweaks, it will greatly improve the developer experience for Sui Unity SDK users.

Thanks again for your contribution! 🚀

Note: If you can share the full content of BuildScript.cs, I can provide more targeted feedback on the implementation details.