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

RTools42 support #645

Merged
merged 58 commits into from
Apr 26, 2022
Merged

RTools42 support #645

merged 58 commits into from
Apr 26, 2022

Conversation

rok-cesnovar
Copy link
Member

Submission Checklist

  • Run unit tests
  • Declare copyright holder and agree to license (see below)

Summary

Closes #637

Will post the final summary and reasons behind the fixes once we can confirm this is it - I am 90% certain that it is, but want a few others to test as well.

Copyright and Licensing

Please list the copyright holder for the work you are submitting
(this will be you or your assignee, such as a university or company):
Rok Češnovar

By submitting this pull request, the copyright holder is agreeing to
license the submitted work under the following licenses:

@rok-cesnovar
Copy link
Member Author

rok-cesnovar commented Apr 17, 2022

TODO:

  • get some additional confirmation this is good for R 4.2 on Windows
  • test that it works with Github Actions
  • do the same processx env trick for Rtools 4.0 (avoid writing to user's .Renviron)
  • bump minor version

@rok-cesnovar rok-cesnovar changed the title [WIP] RTools42 support RTools42 support Apr 23, 2022
@rok-cesnovar
Copy link
Member Author

Everything should now be working for R 4.2 users on Windows. I confirmed this on Github Actions as well as some additional systems I can access. @tjmahr has confirmed that it works (with some warnings/error) in the issue. I was able to replicate his issues and fixed them.

@jgabry do you mind if I merge this assuming tests pass? I would like to make a Stan blog post warning users about what is going on. I bumped the package version as well.

The only really big change is the new withr dependency - each call to processx is now wrapped into a withr() call that makes sure all the right paths are set up. We previously did this by manipulating the PATH with Sys.setenv() and writing to .renviron in the toolchain check. I think this is cleaner and safer now. Do let me know if you disagree.

@rok-cesnovar
Copy link
Member Author

I would like to merge this soon because the 4.2 release is now officially out.

@rok-cesnovar
Copy link
Member Author

rok-cesnovar commented Apr 25, 2022

@jgabry do you mind if I go ahead and merge this PR with the bumped version? R 4.2 has been released and I want to put out a blog post with the news on how to address Stan+Windows+R4.2 issues.

I can rebuild the website once the PR is merged.

@jgabry
Copy link
Member

jgabry commented Apr 26, 2022

Sorry for the delay, I've been away. Fine to merge if you think it's ready!

@rok-cesnovar
Copy link
Member Author

Great, can you let me know what you run to rebuild the website?

@rok-cesnovar rok-cesnovar merged commit 6e0d351 into master Apr 26, 2022
@rok-cesnovar rok-cesnovar deleted the rtools42 branch April 26, 2022 16:04
@jgabry
Copy link
Member

jgabry commented Apr 26, 2022

To rebuild the whole site:

withr::with_envvar(c("NOT_CRAN" = "true"), pkgdown::build_site(run_dont_run = TRUE, seed = 1234))

Do you want to do add an entry to the Releases page too? https://github.com/stan-dev/cmdstanr/releases

Thanks for fixing the windows issues!

@jgabry
Copy link
Member

jgabry commented Apr 28, 2022

I have some time now so I can update the NEWS and add to the releases page

@rok-cesnovar
Copy link
Member Author

Thanks!

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.

Windows & RTools 4.2
3 participants