-
Notifications
You must be signed in to change notification settings - Fork 47
Copy GA DLL for Windows Build and Play Mode if it exists #1271
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
base: main
Are you sure you want to change the base?
Conversation
This change addresses an issue where `analytics_win.dll` was not being included in Windows builds or available during Play Mode when the Firebase Analytics module was present. The following modifications were made: - Modified `editor/app/src/DllLocationPatcher.cs`: - Added logic to `OnPostProcessBuild` to copy `analytics_win.dll` from the project root to the build's plugin directory (`<project_name>_Data/Plugins/`) when building for StandaloneWindows or StandaloneWindows64, but only if the Firebase Analytics module is detected and the DLL exists. - Created `editor/app/src/AnalyticsPlayModeSetup.cs`: - This new editor script subscribes to `EditorApplication.playModeStateChanged`. - When entering Play Mode on a Windows editor, it checks for the Firebase Analytics module and the existence of `analytics_win.dll` in the project root. - If both conditions are met, it copies `analytics_win.dll` to `Assets/Plugins/` to make it available during Play Mode. These changes ensure that `analytics_win.dll` is correctly placed for both Windows builds and Play Mode sessions, facilitating proper Firebase Analytics functionality on the Windows platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @jonsimantov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the Firebase Analytics integration for Windows by ensuring the necessary analytics_win.dll
is correctly placed for both editor Play Mode sessions and standalone Windows builds. This automates the deployment of the critical DLL, improving the developer experience and ensuring proper analytics functionality across the Windows platform.
Highlights
- Play Mode DLL Handling: A new editor script (
AnalyticsPlayModeSetup.cs
) was introduced to automatically copyanalytics_win.dll
toAssets/Plugins/
when entering Play Mode in the Unity editor on Windows, ensuring Firebase Analytics functions correctly during development. - Windows Build DLL Handling: The existing
DllLocationPatcher.cs
was updated with a new post-build process to copyanalytics_win.dll
into the_Data/Plugins/
directory of Windows standalone builds, enabling Firebase Analytics functionality in compiled applications.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request successfully adds functionality to copy the analytics_win.dll
for Windows Build and Play Mode when Firebase Analytics is used. The changes are logical and address the described requirements. Key areas for improvement include enhancing maintainability by introducing shared constants for duplicated string literals (like DLL names, paths, and type names) across the new and modified files, and improving robustness in the build process by adding error handling for file operations. No specific style guide was provided; feedback is based on common C# and Unity development best practices.
…LL copy This commit addresses feedback from the previous review: - Created `FirebaseAnalyticsEditorConstants.cs` to store shared constants for: - `DllName` ("analytics_win.dll") - `DllSourcePath` ("./analytics_win.dll") - `AnalyticsTypeFullName` ("Firebase.Analytics.FirebaseAnalytics, Firebase.Analytics") - Updated `editor/app/src/DllLocationPatcher.cs`: - Replaced hardcoded strings with the new shared constants. - Wrapped file I/O operations (directory creation and DLL copy for Windows builds) in a try-catch block to handle potential exceptions (e.g., IOExceptions, UnauthorizedAccessException) and log specific errors. - Updated `editor/app/src/AnalyticsPlayModeSetup.cs`: - Replaced hardcoded strings and local constants with the new shared constants. - Wrapped file I/O operations (directory creation and DLL copy for Play Mode) in a try-catch block to handle potential exceptions and log specific errors. These changes improve maintainability by centralizing critical strings and enhance robustness by ensuring that file operation errors during build post-processing or Play Mode setup are caught and logged gracefully.
This commit corrects the destination path for `analytics_win.dll` when copied during Play Mode setup on Windows. Previously, the DLL was copied to `Assets/Plugins/`. This has been changed to the project root (`./`). Copying to the project root ensures that the native DLL can be found by the Unity editor executable when using P/Invoke, which typically searches the application's directory. Changes in `editor/app/src/AnalyticsPlayModeSetup.cs`: - The destination directory for copying `analytics_win.dll` in Play Mode is now `"./"` (the project root). - The corresponding log message has been updated. - Removed the `AssetDatabase.Refresh()` call, as it's not necessary for this type of DLL handling in the editor. The build-time copy location in `DllLocationPatcher.cs` remains unchanged as it was already correct for standalone builds.
This commit refines the logic in `AnalyticsPlayModeSetup.cs` to correctly handle the scenario where `analytics_win.dll` (the source) is already located in the project root (the destination for Play Mode). Previously, if the DLL was in the project root, the `File.Copy` operation would attempt to copy the file onto itself, resulting in an `IOException`. Changes in `editor/app/src/AnalyticsPlayModeSetup.cs`: - Before attempting to copy the DLL for Play Mode: - The full canonical paths of the source and destination are now compared. - If the source and destination paths are identical (i.e., the DLL is already in the project root), the copy operation is skipped, and an informational message is logged. - The file copy (with its try-catch block) only proceeds if the source and destination paths are different. This prevents an unnecessary `IOException` and ensures the setup is cleaner when the DLL is already correctly placed for Play Mode.
This commit introduces `Firebase.Editor.App.asmdef` in the `editor/app/src/` directory. The purpose of this assembly definition file is to: 1. Define a distinct assembly named `Firebase.Editor.App` for the editor scripts located in `editor/app/src/` (including `DllLocationPatcher.cs`, `AnalyticsPlayModeSetup.cs`, and `FirebaseAnalyticsEditorConstants.cs`). 2. Ensure this assembly is compiled exclusively for the Unity Editor environment by setting `includePlatforms: ["Editor"]`. 3. Improve code organization and compilation consistency across different operating systems. This change directly addresses a build error reported on Mac (`CS0103: The name 'FirebaseAnalyticsEditorConstants' does not exist in the current context`), which occurred because the editor scripts were not being compiled into a clearly defined assembly, leading to symbol resolution issues on that platform. The new .asmdef file ensures that all relevant editor scripts are compiled together, making `FirebaseAnalyticsEditorConstants` accessible to other scripts within the same `Firebase.Editor.App` assembly during editor script compilation on all platforms.
… the file structure and address a potential build error you encountered on Mac. Specifically, I have: 1. **Removed `.asmdef` from `editor/app/src/`**: The editor scripts in this directory will now be compiled into the default editor assembly. 2. **Removed `FirebaseAnalyticsEditorConstants.cs` and reverted its usage**: * `editor/app/src/DllLocationPatcher.cs` now uses hardcoded strings for DLL names, paths, and type lookups. * `editor/app/src/AnalyticsPlayModeSetup.cs` now uses hardcoded strings for DLL names, paths, and type lookups. The goal here is to make things simpler and resolve the Mac build error (`CS0103: The name 'FirebaseAnalyticsEditorConstants' does not exist in the current context`) by removing the file that seemed to be causing the issue. If the build error continues on your Mac, I'll investigate further with this simplified setup.
This change adds
analytics_win.dll
(if present) to the Windows build directory / Play Mode when the Firebase Analytics module is used.The following modifications were made:
editor/app/src/DllLocationPatcher.cs
:OnPostProcessBuild
to copyanalytics_win.dll
from the project root to the build's plugin directory (<project_name>_Data/Plugins/
) when building for StandaloneWindows or StandaloneWindows64, but only if the Firebase Analytics module is detected and the DLL exists.editor/app/src/AnalyticsPlayModeSetup.cs
:EditorApplication.playModeStateChanged
.analytics_win.dll
in the project root.analytics_win.dll
toAssets/Plugins/
to make it available during Play Mode.These changes ensure that
analytics_win.dll
is correctly placed for both Windows builds and Play Mode sessions, facilitating proper Firebase Analytics functionality on the Windows platform.Description
Testing
Type of Change
Place an
x
the applicable box: