-
-
Notifications
You must be signed in to change notification settings - Fork 443
Remove arm runtime files #3927
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
Remove arm runtime files #3927
Conversation
🥷 Code experts: jjw24 Jack251970 has most 👩💻 activity in the files. See details
Activity based on git-commit:
Knowledge based on git-blame:
Activity based on git-commit:
Knowledge based on git-blame: ✨ Comment |
Be a legend 🏆 by adding a before and after screenshot of the changes you made, especially if they are around UI/UX. |
📝 WalkthroughWalkthroughAdds removal of Windows runtime folders Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant MSB as MSBuild
participant Proj as .csproj Targets
Dev->>MSB: Build
MSB->>Proj: Compile / produce $(OutputPath)
MSB->>Proj: Invoke RemoveUnnecessaryRuntimesAfterBuild
Note right of Proj: Deletes:<br/>`$(OutputPath)runtimes\osx-x64`<br/>`$(OutputPath)runtimes\win-arm`<br/>`$(OutputPath)runtimes\win-arm64`
Dev->>MSB: Publish
MSB->>Proj: Produce publish artifacts at $(PublishDir)
MSB->>Proj: Invoke RemoveUnnecessaryRuntimesAfterPublish
Note right of Proj: Deletes:<br/>`$(PublishDir)runtimes\osx-x64`<br/>`$(PublishDir)runtimes\win-arm`<br/>`$(PublishDir)runtimes\win-arm64`
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Flow.Launcher/Flow.Launcher.csproj (1)
42-64
: DRY the duplicate runtime lists to a single property or Directory.Build.targets.The same long list appears in two targets and across projects. Centralize to reduce drift.
Example (conceptual):
<PropertyGroup> <UnnecessaryRuntimes>browser-wasm;linux-arm;linux-arm64;...;osx;osx-arm64;osx-x64;win-arm;win-arm64;win-x86</UnnecessaryRuntimes> </PropertyGroup> <Target Name="RemoveUnnecessaryRuntimesAfterBuild" AfterTargets="Build"> <RemoveDir Directories="$([System.String]::Join(';', %(UnnecessaryRuntimes.Identity)->'$(OutputPath)runtimes\%(Identity)'))" /> </Target> <Target Name="RemoveUnnecessaryRuntimesAfterPublish" AfterTargets="Publish"> <RemoveDir Directories="$([System.String]::Join(';', %(UnnecessaryRuntimes.Identity)->'$(PublishDir)runtimes\%(Identity)'))" /> </Target>Or move this into Directory.Build.targets for all projects.
Also applies to: 66-88
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (1)
39-61
: Deduplicate shared cleanup logic across projects.Consider centralizing the runtime list/targets to avoid future divergence between the app and plugin.
Happy to provide a Directory.Build.targets draft if useful.
Also applies to: 63-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
Flow.Launcher/Flow.Launcher.csproj
(2 hunks)Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj
(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-08T15:52:58.573Z
Learnt from: taooceros
PR: Flow-Launcher/Flow.Launcher#2616
File: Flow.Launcher/Flow.Launcher.csproj:7-7
Timestamp: 2024-10-08T15:52:58.573Z
Learning: In the Flow Launcher project, the version number in the `Flow.Launcher.csproj` file is dynamically updated during the CI/CD process.
Applied to files:
Flow.Launcher/Flow.Launcher.csproj
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
Flow.Launcher/Flow.Launcher.csproj (1)
84-87
: Verify x86 support stance before deleting win-x86 at publish.If 32-bit Windows is still supported anywhere, dropping win-x86 could regress those users. If x86 is officially unsupported, this is fine.
Would you like me to scan the repo/CI configs for any remaining x86 artifacts or docs references and report back?
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj (1)
81-84
: Confirm x86 deprecation for the plugin as well.Ensure no x86 users or packaging paths remain for BrowserBookmark before removing win-x86 from publish output.
I can grep the repo and pipeline configs to confirm no x86 artifacts are produced or referenced.
Plugins/Flow.Launcher.Plugin.BrowserBookmark/Flow.Launcher.Plugin.BrowserBookmark.csproj
Outdated
Show resolved
Hide resolved
let's see the build size with x86 removed. |
I wonder if |
If we only keep runtime files of ×64, the installer size is 90 MB |
It's requested, but we have had issues with Everything plugin and the fact that the rest of the plugins in the community are not publish arm versions, so for now we should remove ARM runtime files until we figure out a proper path forward for ARM builds. |
Should we keep runtime files for ×86? If not, this PR should be mergable. |
Anyway, we should remove runtimes for arm to decrease size. |
Yeah, let's keep x86 for now. |
Done |
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.
Thank you for this 👍
…d_runtime_files Remove unused runtime files
Following on from #3826, remove ARM runtime files to reduce the artifact size.
Remove arm runtime files:
Note:
With x86 removed- installer is 90mb
With x86 kept- installer is 95mb