Repository navigation
Add CI support for publishing Windows ARM64 wheels - #14
MugundanMCW wants to merge 2 commits into
Conversation
HugoMVale
left a comment
There was a problem hiding this comment.
Thanks for the PR.
Please have a look at the request below.
| $LlvmBin = 'C:\Program Files\LLVM\bin' | ||
| $LlvmInstaller = "$env:RUNNER_TEMP\LLVM-22.1.8-woa64.exe" | ||
| $LlvmUrl = "https://github.com/llvm/llvm-project/releases/download/llvmorg-$LlvmVersion/LLVM-$LlvmVersion-woa64.exe" | ||
| Invoke-WebRequest $LlvmUrl -UseBasicParsing -OutFile $LlvmInstaller |
There was a problem hiding this comment.
No hash verification for downloaded binaries (High Risk).
- Lines 7 and 28 download executables and archives without verifying checksums
- An attacker performing a MITM attack or compromising GitHub's release infrastructure could inject malicious code
- Fix: Validate SHA256 hashes against known good values
There was a problem hiding this comment.
Thanks for the review, I have added the hash verification for llvm. Kindly take a look and let me know if you have any further queries
Updated LLVM download URL and added SHA256 verification.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
=======================================
Coverage 99.59% 99.59%
=======================================
Files 4 4
Lines 244 244
Branches 63 63
=======================================
Hits 243 243
Partials 1 1 |
|
I finally did what I should have done a couple of years ago, and measured the run times with reference blas and with openblas. The difference is insignificant, so it makes no sense to carry the burden of figuring out how to install openblas for every os/arch now and in the future. This interferes with your PR -- sorry for that -- but should ultimately make things simpler. We just need the arch extension, not the openblas install. Can you please take care of it? |
PR Description: