Skip to content
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

Windows GHA workflow #175

Merged
merged 23 commits into from
Dec 12, 2023
Merged

Windows GHA workflow #175

merged 23 commits into from
Dec 12, 2023

Conversation

DavisVaughan
Copy link
Contributor

@DavisVaughan DavisVaughan commented Dec 7, 2023

  • Delete temporary 0.1.32 Release before merging

General idea is:

  • build_macos to build the Mac ark binaries (arm64 and intel)
  • build_windows to build the Windows ark.exe (only x64 for now, likely arm in the future?)
  • create_release bundles the modules/ into the Mac and Windows specific zip files

We get artifacts on the tags like:

  • ark-0.1.31-windows-x64.zip
  • ark-0.1.31-debug-windows-x64.zip

I've left in the x64 here to go along with the universal of darwin-universal in our Mac builds. I assume in the future if we need ARM windows builds then we will either have a universal build that replaces this, or an additional -arm64 artifact

Comment on lines -169 to +170
let source = format!("{}/src/modules", env!("CARGO_MANIFEST_DIR"));
root = Path::new(&source).to_path_buf();
let source = env!("CARGO_MANIFEST_DIR");
root = Path::new(&source).join("src").join("modules").to_path_buf();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed a bug that only showed up in release builds. Need \ on Windows!

- name: Setup R
uses: r-lib/actions/setup-r@v2
with:
r-version: '4.3.2'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are going to eventually have to decide what our minimum supported R version is, and then build libR-sys against that.

Then for any C utilities that comes from a newer version of R than that, we would dynamically load those out of the user's R.dll at runtime using dlopen or the Windows equivalent. RStudio seems to take this approach in a few places (For example, with R_DefParamsEx(), introduced in R 4.0.0 ish but R really wants you to use that now on startup)

If libR-sys doesn't support that version of R that we pin against, we may have to maintain our own bindings 😢

But otherwise what currently happens is that on Windows we probably can't load up R 4.0.0 or earlier right now due to us using some utilities that came from R versions after that (even if we guard against using those with a runtime R version check, that isn't enough, because the linker needs to resolve the symbols at runtime and it can't)

@DavisVaughan DavisVaughan changed the title Windows GHA workflow (WIP) Windows GHA workflow Dec 11, 2023
@DavisVaughan DavisVaughan marked this pull request as ready for review December 11, 2023 17:35
Comment on lines -14 to +15
# We try to dynamically look up the year (2022) and exact version (14.37.32822)
# in case they change on us
# A number of the pieces are unstable (year, Community vs Enterprise, exact version)
# so we try and use `vswhere.exe` and `vsdevcmd.bat` to find the exact path
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can mostly ignore the specifics of the changes in this file. I thought I had it right before, but that was with a Community version of the vs tools, and the Enterprise version is on the runner.

Anyways, I think it should be more robust now through the use of the "official" vswhere utility

@DavisVaughan DavisVaughan merged commit 97708ae into main Dec 12, 2023
1 check passed
@DavisVaughan DavisVaughan deleted the feature/windows-github-workflow branch December 12, 2023 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants