ARM64 Build #2985
ARM64 Build #2985
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve updates to multiple files to support the ARM64 architecture. The Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
appveyor.yml (1)
33-33: LGTM! Please remove the trailing spaces.The addition of the test script is a good enhancement to the build process. It ensures that the tests are run against the
Releasebuild without unnecessary rebuilding.Please remove the trailing spaces in this line to maintain clean code.
Apply this diff to remove the trailing spaces:
- +Tools
yamllint
[error] 33-33: trailing spaces
(trailing-spaces)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- Flow.Launcher/Properties/PublishProfiles/Net7.0-SelfContained.pubxml (1 hunks)
- appveyor.yml (1 hunks)
Additional context used
yamllint
appveyor.yml
[error] 33-33: trailing spaces
(trailing-spaces)
Additional comments not posted (2)
Flow.Launcher/Properties/PublishProfiles/Net7.0-SelfContained.pubxml (1)
9-9: LGTM! The changes are consistent with the PR objective.The modifications to the
<Platform>and<RuntimeIdentifier>properties correctly target the ARM64 architecture for the build and runtime environment.However, please ensure thorough testing of the application on ARM64 devices to validate compatibility and performance. Consider running the following manual tests:
- Install the application on an ARM64 Windows device and verify successful installation.
- Launch the application and perform basic functionality tests to ensure core features work as expected.
- Conduct performance tests to measure application responsiveness and resource utilization on the ARM64 device.
- Test edge cases and boundary conditions that may be impacted by the architecture change.
Documenting the results of these tests will help build confidence in the ARM64 support.
Also applies to: 12-12
appveyor.yml (1)
26-26: Verify compatibility with ARM64 architecture.The platform change from "Any CPU" to "ARM64" aligns with the PR objective. Please ensure that the rest of the project is compatible with ARM64 architecture.
Run the following script to verify the project's compatibility with ARM64 architecture:
- Specify ARM64 target - Everything Search will return an error
This comment has been minimized.
This comment has been minimized.
|
huh I think the CI is problematic; we need to switch to github action for this https://help.appveyor.com/discussions/questions/62461-build-on-windows-arm64-possible |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (3)
Flow.Launcher/Flow.Launcher.csproj (2)
41-43: Review of ARM64 Debug ConfigurationThe addition of the ARM64 Debug configuration is a crucial step for supporting ARM64 architecture. The use of
$(DefineConstants)ensures that existing constants are not overwritten but extended, which is a good practice to maintain compatibility and extend functionality.However, it's important to ensure that other necessary configurations specific to ARM64, such as
PlatformTargetand potentially different optimization settings or output paths, are also considered. This would ensure that the ARM64 builds are correctly configured and optimized for that architecture.Consider adding more ARM64-specific settings such as
PlatformTargetandOptimizeto fully support and optimize the ARM64 build process.
44-46: Review of ARM64 Release ConfigurationSimilar to the Debug configuration, the Release configuration for ARM64 correctly extends the existing constants. This inclusion is essential for ensuring that ARM64-specific code can be conditionally compiled.
As with the Debug configuration, it would be beneficial to review and possibly include additional ARM64-specific settings. Release builds often have different optimization and debugging settings that could be crucial for performance on ARM64 devices.
Recommend reviewing and adding ARM64-specific settings in the Release configuration, such as
PlatformTargetandOptimize, to enhance performance and compatibility.Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingSearchManager.cs (1)
23-30: Conditional ARM64 check is well-implemented.The addition of the ARM64 check in
ThrowIfEverythingNotAvailableAsyncmethod is crucial for ensuring that the application does not attempt to use features that are not supported on ARM64 architectures. The use ofEngineNotAvailableExceptionis appropriate and aligns with the application's error handling strategy.Consider adding a configuration or feature flag system in the future to manage such platform-specific checks more dynamically, which could enhance scalability and maintainability.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- .github/workflows/dotnet.yml (1 hunks)
- Flow.Launcher/Flow.Launcher.csproj (1 hunks)
- Plugins/Flow.Launcher.Plugin.Explorer/Languages/en.xaml (1 hunks)
- Plugins/Flow.Launcher.Plugin.Explorer/Main.cs (2 hunks)
- Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingSearchManager.cs (4 hunks)
- Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchManager.cs (1 hunks)
Files skipped from review due to trivial changes (1)
- Plugins/Flow.Launcher.Plugin.Explorer/Search/SearchManager.cs
Additional comments not posted (15)
Plugins/Flow.Launcher.Plugin.Explorer/Main.cs (3)
45-45: Approved formatting change.The addition of a blank line improves readability and does not impact functionality.
48-51: Approved conditional compilation for ARM64 compatibility.The use of
#if !ARM64to conditionally load theEverythingApiDllImportis a smart approach to ensure compatibility with ARM64 architectures. This prevents potential runtime errors due to incompatibility of the DLL with ARM64.
80-80: Approved minor formatting change in error handling.The minor change in formatting does not alter the functionality but helps in maintaining code consistency. The specific handling of
EngineNotAvailableExceptionensures that users receive meaningful feedback in case of errors, which is crucial for user experience..github/workflows/dotnet.yml (8)
4-15: Workflow Triggers and NamingThe workflow is well-named and clearly triggers on appropriate events such as
workflow_dispatch,push, andpull_requestfordevandmasterbranches. This setup ensures that the workflow runs on all important code changes and manual triggers, which is suitable for continuous integration practices.
20-24: Environment Variables SetupThe environment variables
FlowVersion,NUGET_CERT_REVOCATION_MODE, andBUILD_NUMBERare correctly set up. These variables are crucial for versioning and ensuring that the NuGet operations work as expected in offline mode, which is particularly useful in CI environments where internet access might be restricted.
26-33: Project Versioning StepThe use of
vers-one/dotnet-project-version-updaterto update the version inSolutionAssemblyInfo.csis a smart choice to automate version management. This step ensures that each build can be uniquely identified using theFlowVersionandBUILD_NUMBER, which is essential for traceability and release management.
34-49: Cache Restoration StepsThe configuration for restoring the NuGet and .NET tool caches is well-implemented. Using paths like
~/.nuget/packagesand~/.dotnet/toolsalong with keys based on the operating system and file hashes ensures efficient and reliable cache usage, which can significantly speed up the build process by reusing previously downloaded packages and tools.
55-60: Conditional Tool InstallationThe step to conditionally install the
vpktool only if it is not already present is a good practice to avoid unnecessary reinstalls. This check minimizes the setup time and ensures that the latest tool version is used without affecting the existing setup.
67-68: Service Initialization for TestingInitializing the Windows Search service is necessary for running tests that depend on this service. The commands to set the service to start automatically and to start it are correctly placed before the testing steps, ensuring that all prerequisites for the tests are met.
71-73: Post-Build Script ExecutionExecuting a PowerShell script for post-build tasks with parameters like
flowversionandbuild numberis a good practice. This allows for custom operations such as cleanup, notifications, or additional checks to be performed after the main build and test steps.
74-109: Artifact Upload StepsThe steps to upload various build artifacts like the plugin NuGet package, installer executable, portable version, full NuGet package, and release information are well-configured. Using
actions/upload-artifactwith specific paths and settingcompression-levelto 0 (for faster uploads at the cost of larger file size) is appropriate for these artifacts. This setup ensures that all relevant outputs from the build process are stored and available for further actions like deployment or manual downloads.Plugins/Flow.Launcher.Plugin.Explorer/Search/Everything/EverythingSearchManager.cs (3)
54-56: Formatting improvements enhance readability.The changes in formatting within the
ClickToInstallEverythingAsyncmethod improve readability and maintainability by breaking longer lines into more manageable segments. This aligns well with common coding standards.
71-82: Method signature reformatting inSearchAsyncenhances clarity.The changes to the method signature and parameter alignment in
SearchAsyncenhance clarity and readability. The use of theEnumeratorCancellationattribute is correctly applied, and the overall formatting aligns with best practices for long method signatures.
125-126: Method signature reformatting inEnumerateAsyncenhances clarity.The changes to the method signature and parameter alignment in
EnumerateAsyncenhance clarity and readability. The use of theEnumeratorCancellationattribute is correctly applied, and the overall formatting aligns with best practices for long method signatures.Plugins/Flow.Launcher.Plugin.Explorer/Languages/en.xaml (1)
130-130: Approve the addition of the new string resource.The new string resource
flowlauncher_plugin_everything_arm64_not_supportedis correctly added and clearly communicates the limitation of ARM64 support for the Everything plugin. This is a good practice in user communication, ensuring that users are aware of compatibility issues.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…299A475544173E6DAECFEF6ACC0D46C5.txt
Add ARM64 Support for EverythingSDK
|
ARM64 Everything SDK dll from #2996 |
This comment has been minimized.
This comment has been minimized.
|
TODO: add the code related to import arm64 everything dll |
|
@taooceros Thanks for putting this together. Glad we're getting an arm64 build. When can we get this merged and released? What else does the team still need to do? |
There are quite a few reason that it is not yet production ready.
Those issues are non-trivial, and given that ARM64 users are still small, I would encourage people who wants ARM64 native to download the artifact from this pr. |
@check-spelling-bot Report🔴 Please reviewSee the 📂 files view, the 📜action log, or 📝 job summary for details.
See ❌ Event descriptions for more information. If the flagged items are 🤯 false positivesIf items relate to a ...
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
appveyor.yml (2)
33-33: Remove trailing whitespaceThere is trailing whitespace on line 33.
- +🧰 Tools
🪛 yamllint (1.35.1)
[error] 33-33: trailing spaces
(trailing-spaces)
Line range hint
38-83: Consider platform-specific artifact handlingThe current artifact configuration doesn't distinguish between x64 and ARM64 builds. This could lead to confusion when both architectures are supported.
Consider updating the artifact paths and names to include the platform:
-- path: 'Output\Packages\Flow-Launcher-*.exe' - name: Squirrel Installer +- path: 'Output\Packages\Flow-Launcher-$(platform)-*.exe' + name: Squirrel Installer ($(platform))🧰 Tools
🪛 yamllint (1.35.1)
[error] 33-33: trailing spaces
(trailing-spaces)
[warning] 35-35: wrong indentation: expected 0 but found 2
(indentation)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
Plugins/Flow.Launcher.Plugin.Explorer/EverythingSDK/arm64/Everything.dllis excluded by!**/*.dll
📒 Files selected for processing (2)
Flow.Launcher/Flow.Launcher.csproj(1 hunks)appveyor.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Flow.Launcher/Flow.Launcher.csproj
🧰 Additional context used
🪛 yamllint (1.35.1)
appveyor.yml
[error] 33-33: trailing spaces
(trailing-spaces)
🔇 Additional comments (1)
appveyor.yml (1)
34-34: Consider platform-specific test configuration
While adding tests is good, ensure that the test command accounts for ARM64-specific scenarios. You might need to:
- Skip tests that are not compatible with ARM64
- Add platform-specific test configurations
Run this script to check for any ARM64-specific test configurations or platform checks in the test files:
| @@ -23,13 +23,14 @@ assembly_info: | |||
| assembly_informational_version: $(flowVersion) | |||
|
|
|||
| image: Visual Studio 2022 | |||
| platform: Any CPU | |||
| platform: ARM64 | |||
There was a problem hiding this comment.
Critical: AppVeyor does not support ARM64 builds
Setting the platform to ARM64 in AppVeyor configuration is problematic as AppVeyor does not provide native ARM64 build support. This could lead to build failures or incorrect artifacts.
As mentioned in the PR discussion, consider migrating to GitHub Actions for proper ARM64 build support.
Consider:
- Creating a separate GitHub Actions workflow for ARM64 builds
- Using AppVeyor only for x64 builds until the migration is complete
- Adding platform-specific naming for artifacts to distinguish between x64 and ARM64 builds
No description provided.