-
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
Changes from all commits
76e1642
5b8eaa2
67891d1
a88afb4
7bedc2e
563ac03
9a8eb3a
3f20e3c
3f1460d
458e17e
25fcb64
a651278
69af463
c597089
a1c372b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,124 @@ | ||||||
| get_r_version <- function() { | ||||||
| R.version | ||||||
| } | ||||||
|
Comment on lines
+1
to
+3
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a trade off for testability. Depending on static state (
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no practical different between
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and so that you can unit test it |
||||||
|
|
||||||
| is_windows_arm <- function() { | ||||||
| proc_arch <- Sys.getenv("PROCESSOR_ARCHITECTURE") | ||||||
| r_arch <- get_r_version()[["arch"]] | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same reason as above |
||||||
|
|
||||||
| if (identical(proc_arch, "ARM64") && !identical(r_arch, "aarch64")) { | ||||||
| cli::cli_abort( | ||||||
| c( | ||||||
| "Architecture mismatch detected.", | ||||||
| "i" = "You are running the {.code {proc_arch}} version of Windows, but the {.code {r_arch}} version of R.", | ||||||
| "i" = "You can find ARM64 build of R at {.url https://www.r-project.org/nosvn/winutf8/aarch64}" | ||||||
| ), | ||||||
| class = "rextendr_error" | ||||||
| ) | ||||||
| } | ||||||
|
|
||||||
| identical(proc_arch, "ARM64") && identical(r_arch, "aarch64") | ||||||
| } | ||||||
|
|
||||||
| throw_if_no_rtools <- function() { | ||||||
| if (!suppressMessages(pkgbuild::has_rtools())) { | ||||||
| cli::cli_abort( | ||||||
| c( | ||||||
| "Unable to find Rtools that are needed for compilation.", | ||||||
| "i" = "Required version is {.emph {pkgbuild::rtools_needed()}}." | ||||||
| ), | ||||||
| class = "rextendr_error" | ||||||
| ) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| throw_if_not_ucrt <- function() { | ||||||
| if (!identical(get_r_version()[["crt"]], "ucrt")) { | ||||||
| cli::cli_abort( | ||||||
| c( | ||||||
| "R must be built with UCRT to use rextendr.", | ||||||
| "i" = "Please install the UCRT version of R from {.url https://cran.r-project.org/}." | ||||||
| ), | ||||||
| class = "rextendr_error" | ||||||
| ) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| get_rtools_version <- function() { | ||||||
| minor_patch <- package_version(get_r_version()[["minor"]]) | ||||||
|
|
||||||
| if (minor_patch >= "5.0") { | ||||||
| "45" | ||||||
| } else if (minor_patch >= "4.0") { | ||||||
| "44" | ||||||
| } else if (minor_patch >= "3.0") { | ||||||
| "43" | ||||||
| } else { | ||||||
| "42" | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| 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) | ||||||
| } | ||||||
|
Comment on lines
+61
to
+75
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😅
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this differ from rtools bin path funciton?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Path to rtools cargo installation, different subfolder
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, okay! So rtools now distributes cargo??!
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||||||
| ) | ||||||
| } | ||||||
|
Comment on lines
+77
to
+94
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the difference between this and
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||||||
| } | ||||||
|
Comment on lines
+96
to
+105
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the difference between this and |
||||||
|
|
||||||
| use_rtools <- function(.local_envir = parent.frame()) { | ||||||
| throw_if_no_rtools() | ||||||
| throw_if_not_ucrt() | ||||||
|
Comment on lines
+108
to
+109
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_arm <- is_windows_arm() | ||||||
| rtools_version <- get_rtools_version() | ||||||
| rtools_home <- get_rtools_home(rtools_version, is_arm) | ||||||
| rtools_bin_path <- get_rtools_bin_path(rtools_home, is_arm) | ||||||
|
|
||||||
| withr::local_path(rtools_bin_path, action = "suffix", .local_envir = .local_envir) | ||||||
|
|
||||||
| if (is_arm) { | ||||||
| cargo_path <- get_path_to_cargo_folder_arm(rtools_home) | ||||||
| withr::local_path(cargo_path, .local_envir = .local_envir) | ||||||
| } | ||||||
|
|
||||||
| invisible() | ||||||
| } | ||||||
Uh oh!
There was an error while loading. Please reload this page.