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

Possibly unintentional breakage on 32 bit Windows and R 3.6 #501

Closed
DavisVaughan opened this issue Nov 16, 2023 · 7 comments
Closed

Possibly unintentional breakage on 32 bit Windows and R 3.6 #501

DavisVaughan opened this issue Nov 16, 2023 · 7 comments

Comments

@DavisVaughan
Copy link
Contributor

I see there is a new stringi release and I happened to just note that our R 3.6 Windows build is failing due to an inability to build stringi from source in the 32 bit checks:
https://github.com/tidyverse/dplyr/actions/runs/6896097548/job/18761489770?pr=6967#step:5:4120

  *** arch - i386
  Error: Error: package or namespace load failed for 'stringi' in library.dynam(lib, package, package.lib):
   DLL 'stringi' not found: maybe not installed for this architecture?

I think the relevant lines are:

stri_container_utf8.o -L. -licu_common -licu_i18n -licu_stubdata -LC:/R/bin/i386 -lR
  ./libicu_i18n.a(windtfmt.o):windtfmt.cpp:(.text+0x109d): undefined reference to `_imp__ResolveLocaleName@12'
  ./libicu_i18n.a(winnmfmt.o):winnmfmt.cpp:(.text+0xbf7): undefined reference to `_imp__ResolveLocaleName@12'
  collect2.exe: error: ld returned 1 exit status
  installing via 'install.libs.R' to C:/Users/RUNNER~1/AppData/Local/Temp/Rtmpwzkw9c/pkg-lib72c17c12928/stringi
  Warning in file.copy(libfile, dest, overwrite = TRUE) :
    problem copying .\stringi.dll to C:\Users\RUNNER~1\AppData\Local\Temp\Rtmpwzkw9c\pkg-lib72c17c12928\stringi\libs\i386\stringi.dll: No such file or directory
  icu74/data/icudt74l.dat.xz exists
  decompressing icu74/data/icudt74l.dat.xz to: C:/Users/RUNNER~1/AppData/Local/Temp/Rtmpwzkw9c/pkg-lib72c17c12928/stringi/libs
  icu74/data/icudt74l.dat installed successfully

I didn't see a note in the NEWS about this, so just wanted to check if this was intentional or not

(In the tidyverse we tend to try and support 5 minor versions of R, so that is why we still have R 3.6 checks running)

@DavisVaughan
Copy link
Contributor Author

DavisVaughan commented Nov 16, 2023

@jeroen has confirmed this affects R 4.0 and 4.1 with 32 bit Windows as well (we don't explicitly run those checks)

At 4.2 R dropped support for 32 bit Windows, which is possibly why CRAN probably didn't detect this

@jeroen
Copy link
Contributor

jeroen commented Nov 16, 2023

It looks like the ResolveLocaleName Windows API is not available on 32-bit mingw. In the previous release of stringi, the bundled ICU was patched and this API call was conditioned on the macro #ifndef U_STRINGI_PATCHES (probably to fix this issue):

https://github.com/gagolews/stringi/blob/e4cf3176bc3943e6c477885be3445cbbd7d4bab6/src/icu69/i18n/windtfmt.cpp#L98C1-L162

So if I suppose if we would want to fix this, we could backport this hack that avoids the call to ResolveLocaleName at least on 32-bit Windows.

gagolews added a commit that referenced this issue Nov 18, 2023
gagolews added a commit that referenced this issue Nov 18, 2023
gagolews added a commit that referenced this issue Nov 18, 2023
gagolews added a commit that referenced this issue Nov 18, 2023
gagolews added a commit that referenced this issue Nov 18, 2023
gagolews added a commit that referenced this issue Nov 18, 2023
gagolews added a commit that referenced this issue Nov 20, 2023
gagolews added a commit that referenced this issue Nov 21, 2023
gagolews added a commit that referenced this issue Nov 21, 2023
gagolews added a commit that referenced this issue Nov 21, 2023
gagolews added a commit that referenced this issue Nov 21, 2023
gagolews added a commit that referenced this issue Nov 21, 2023
gagolews added a commit that referenced this issue Nov 21, 2023
gagolews added a commit that referenced this issue Nov 21, 2023
gagolews added a commit that referenced this issue Nov 21, 2023
@gagolews
Copy link
Owner

I was able to fix this on my local 32-bit Windows VM, but it's still failing to install via GH actions.

@jeroen any chance you could help me debug it?

This job fails with the rather cryptic message:

/usr/bin/sh: -c: line 8: syntax error: unexpected end of file
make: *** [C:/R/share/make/winshlib.mk:13: stringi.dll] Error 2

@jeroen
Copy link
Contributor

jeroen commented Nov 21, 2023

I'm not sure why the CI fails. But R-3.6 included a very old version of bash and make, that have problems with some paths and long commands.

Honestly I would not bother too much. Nobody uses R-3.6 anymore so you can just bump the CI version to R-4.0 as you already do in order to make sure that 32-bit works. I confirmed manually that R-3.6 does in fact work, just the CI does not.

@gagolews
Copy link
Owner

Thanks for digging into this.

Do you suggest adding Depends: R (>= 4.0) to DESCRIPTION or just leave as-is, which will result in a few other developers' GH CI builds' failing?

@jeroen
Copy link
Contributor

jeroen commented Nov 21, 2023

Just leave as-is, but stop actively testing R-3.6-Windows it in CI. But we don't want to prevent people from installing stringi on R 3.6, especially on Linux, where some people do run very old versions of R.

@gagolews
Copy link
Owner

Makes sense. Thanks again!

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

No branches or pull requests

3 participants