search

conda-forge / ctng-compilers-feedstock

A conda-smithy repository for ctng-compilers.
BSD 3-Clause "New" or "Revised" License
12 stars 28 forks source link

tzinfo patch broken on windows #150

Open apmorton opened 3 months ago

apmorton commented 3 months ago

Solution to issue cannot be found in the documentation.

Issue

The tzinfo patch (https://github.com/conda-forge/ctng-compilers-feedstock/blob/main/recipe/patches/0003-patch-zoneinfo_dir_override-to-point-to-our-tzdata.patch) calls GetModuleFileNameW and memcpy's the result into a char*.

This is an incorrect conversion from wchar_t to char that results in the patch incorrectly returning only the first character (which happens to be the drive letter).

Installed packages

N/A

Environment info

N/A
h-vetinari commented 3 months ago

Thanks for the report. Could you open a PR that fixes it, or suggest how the implementation should look like? If you have a package/feedstock that exercises this code path, we could also add it as a downstream test here in some form.

apmorton commented 3 months ago

I discovered this while working on: https://github.com/conda-forge/howardhinnant_date-feedstock/blob/main/recipe/patches/0001-Locate-zoneinfo-based-on-install-prefix.patch

The "fix" I applied in that patch was to call GetModuleFileNameA. That seemed appropriate in that library since it didn't really look like any of the downstream code would correctly handle wchar paths anyway, but that may not be an appropriate fix for the STL.

I'm also not sure of the context in which libstdc++ is used on win32 in conda-forge?

h-vetinari commented 3 months ago

I'm also not sure of the context in which libstdc++ is used on win32 in conda-forge?

We just introduced tzdb support (https://github.com/conda-forge/ctng-compilers-feedstock/pull/142), and also the mingw-stack is pretty new (libstcxx is not our default C++ stdlib on windows; in fact, it's not ABI-compatible with the STL, so we have to be very careful to use it).

That said, if it's just renaming the function, then that's a very easy fix; you can even rename it directly in the patch (+ increment the build number).