- 
                Notifications
    You must be signed in to change notification settings 
- Fork 33
Support win arm64 in rust source #481
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
Support win arm64 in rust source #481
Conversation
| I will try to add proper unit tests later. The only remaining question is should I retire 32bit and pre-ucrt support. | 
| Can you elaborate on how this PR is supposed to work? I'm quite confused. The  | 
| get_r_version <- function() { | ||
| R.version | ||
| } | 
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.
Given that this just calls a constant value, can we remove this in favor of R.version so that we don't have to tease out the indirection?
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.
This is a trade off for testability. Depending on static state (R.version) is suboptimal
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.
There is no practical different between get_r_version() and R.version in this use case. The function's sole purpose is to grab R.version but with one level of ambiguity.
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.
Yes, and so that you can unit test it
| throw_if_no_rtools() | ||
| throw_if_not_ucrt() | 
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.
since these functions are only used once, would you mind either defining subroutines in the function or removing the function calls for the body calls instead?
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.
No, this is the sole purpose of this: stop writing notebook style code with functions spanning hundreds of rows, with deep nesting.
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.
I think that would make sense if the function was utilized in more than one place. Since it isn't it has the effect of obfuscating the function definition and code making it more challenging for others to contribute or understand the logic of the function.
|  | ||
| is_windows_arm <- function() { | ||
| proc_arch <- Sys.getenv("PROCESSOR_ARCHITECTURE") | ||
| r_arch <- get_r_version()[["arch"]] | 
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.
| r_arch <- get_r_version()[["arch"]] | |
| r_arch <- R.version[["arch"]] | 
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.
Same reason as above
| get_path_to_cargo_folder_arm <- function(rtools_root) { | ||
| path_to_cargo_folder <- file.path(rtools_root, "clangarm64", "bin") | ||
| path_to_cargo <- file.path(path_to_cargo_folder, "cargo.exe") | ||
| if (!file.exists(path_to_cargo)) { | ||
| cli::cli_abort( | ||
| c( | ||
| "{.code rextendr} on ARM Windows requires an ARM-compatible Rust toolchain.", | ||
| "i" = "Check this instructions to set up {.code cargo} using ARM version of RTools: {.url https://github.com/r-rust/faq?tab=readme-ov-file#does-rust-support-windows-on-arm64-aarch64}." # nolint: line_length_linter | ||
| ), | ||
| class = "rextendr_error" | ||
| ) | ||
| } | ||
|  | ||
| normalizePath(path_to_cargo_folder, mustWork = TRUE) | ||
| } | 
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.
Can we add internal doc comments here? I don't necessarily follow what this function does 😅
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.
How does this differ from rtools bin path funciton?
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.
Path to rtools cargo installation, different subfolder
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.
Ah, okay! So rtools now distributes cargo??!
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.
No, but you can install cargo msys2 style on Arm machine because the target we need is not yet supported (Tier 2).
| get_rtools_home <- function(rtools_version, is_arm) { | ||
| env_var <- if (is_arm) { | ||
| sprintf("RTOOLS%s_AARCH64_HOME", rtools_version) | ||
| } else { | ||
| sprintf("RTOOLS%s_HOME", rtools_version) | ||
| } | ||
|  | ||
| default_path <- if (is_arm) { | ||
| sprintf("C:\\rtools%s-aarch64", rtools_version) | ||
| } else { | ||
| sprintf("C:\\rtools%s", rtools_version) | ||
| } | ||
|  | ||
| normalizePath( | ||
| Sys.getenv(env_var, default_path), | ||
| mustWork = TRUE | ||
| ) | ||
| } | 
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.
What is the difference between this and pkgbuild::rtools_path()?
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.
It clones old behavior. Pkgbuild is not 100% reliable
| get_rtools_bin_path <- function(rtools_home, is_arm) { | ||
| # c.f. https://github.com/wch/r-source/blob/f09d3d7fa4af446ad59a375d914a0daf3ffc4372/src/library/profile/Rprofile.windows#L70-L71 # nolint: line_length_linter | ||
| subdir <- if (is_arm) { | ||
| c("aarch64-w64-mingw32.static.posix", "usr") | ||
| } else { | ||
| c("x86_64-w64-mingw32.static.posix", "usr") | ||
| } | ||
|  | ||
| normalizePath(file.path(rtools_home, subdir, "bin"), mustWork = TRUE) | ||
| } | 
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.
What is the difference between this and pkgbuild::rtools_path()? Should we prefer pkgbuild here?
| At its current state, this PR is not worth the feature it offers. There are 400 lines of code, plus the addition of two dependencies, to facilitate a compilation flag. That's just not worth it. We have to have a discussion about the purpose of rextendr, and how it's written. Rextendr have shockingly few contributors, a code base with more functions than features, and a general stinch of no attention given to the dx nor ux. | 
| Disagree on the first point, agree on the second. Feel free to switch the maintainer to another willing soul to improve maintenance. | 
| Thanks for the hard work Ilia. I'll search for another caretaker of your work. And any time you feel like jumping on again, the door is always open. | 
| Codecov Report❌ Patch coverage is  
 Additional details and impacted files
 🚀 New features to boost your workflow:
 | 
This is a draft of a PR to add native support for ARM64 on Windows.
It also drops support for
RTOOLS40, non-ucrtR and 32 bit architecture (subject to debate).