GH-49866: [R][Release] Restore using tzdb on Windows for tzdata#49867
GH-49866: [R][Release] Restore using tzdb on Windows for tzdata#49867amoeba merged 8 commits intoapache:mainfrom
Conversation
This reverts commit f297433.
|
I'm using CI to test a potential fix, c1b8c30 should recreate an environment where the runner doesn't have tzdata and the package can't handle it. |
|
Removing the tzdata install step caused the expected failures: but also a ton more, such as, So that's fun. Went from 3 identical test failures to 60 total test failures. See https://github.com/apache/arrow/actions/runs/24970086455/job/73112987124?pr=49867. |
This reverts commit c1b8c30.
|
Testing my potential fix in 89a6ed9. Expecting CI could pass. |
|
Those extra failures look like they are from us setting that funky TZ in CI to confirm we are doing the right thing. But presumably the CRAN machines will have a more reasonable TZ 😝 In seriousness though, if we need to make that timezone something else that matches the TZ databases, that's ok. We tried to find one that had very few people in it (and had an odd-hour shift) but I don't think we've ever had issues where we had a confluence of coincidental timezone matching and a developer stuck, so maybe we don't need to be so cute about that... |
|
Thanks for taking a look. The fix here seems like it worked, at least in our CI. I'll mark this as ready for review and try it on Winbuilder in a bit. |
This reverts commit 88dc550.
jonkeane
left a comment
There was a problem hiding this comment.
Thanks for this. This looks ok to me, a few small additions to the initialization flow. I might not be familiar enough with C++ build system for R and windows to know this off the top of my head, but this will in effect impact all windows users of the arrow R package, right?
| tzdb::tzdb_initialize() | ||
| set_timezone_database(tzdb::tzdb_path("text")) |
There was a problem hiding this comment.
Should we wrap this in a try/catch and warn if this goes wrong? It would effectively be the same as if they didn't have it, so it might be nice to not block folks from using arrow at all.
There was a problem hiding this comment.
I think that's a good idea, I'll add it.
There was a problem hiding this comment.
Done in deb3bc6. I did it as a packageStartupMessage which felt right.
Co-authored-by: Jonathan Keane <jkeane@gmail.com>
|
@github-actions crossbow submit r-binary-packages |
|
Revision: deb3bc6 Submitted crossbow builds: ursacomputing/crossbow @ actions-b895c1045c
|
|
Hi @jonkeane, Re: your question above: Yes, this essentially effects all Windows users because CRAN's building under MinGW and most Windows users will be using the CRAN binary. With our latest CRAN Windows binary, I get: |
|
PR title and body updated to better reflect the change. |
|
Crossbow failure is fine to ignore. |
Rationale for this change
Winbuilder is currently failing for the tip of the
maint-24.0.0-rbranch. #48601 introduced a new mechanism for tzdata resolution for Arrow C++ that requires non-MSVC Windows users to set up the tzdata database on their own.We need to restore the mechanism we had before to which uses the tzdb package to provide a tzdata database because all CRAN builds are MinGW (GNU) builds.
This change was cherry-picked onto the R 24.0.0 maint branch to avoid any disruption.
What changes are included in this PR?
Are these changes tested?
Yes. Here in CI.
Are there any user-facing changes?
No.