tzdb integration #142

h-vetinari · 2024-06-28T02:31:14Z

Retry bits that were de-scoped from #133

conda-forge-webservices · 2024-06-28T02:31:22Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

This reverts commit 2c40af6.

jjerphan

A first quick review.

jjerphan · 2024-06-28T11:21:57Z

recipe/patches/0004-patch-zoneinfo_dir_override-to-point-to-our-tzdata.patch

+        this_lib = (char*)malloc(total_length + 1);
+        memcpy(this_lib, resolved, total_length);
+#endif
+


For extra safety?

Suggested change

+

+

+ this_lib[static_cast<size_t>(total_length)] = '\0';

It's impossible to have a conda environment where the /lib folder does not have at least depth 1 (and thus a guaranteed null byte that we'll insert below). Not sure what the extra null byte would achieve

jjerphan · 2024-06-28T11:22:13Z

recipe/patches/0004-patch-zoneinfo_dir_override-to-point-to-our-tzdata.patch

+    if (dladdr(addr, &info)) {
+      char* resolved = realpath(info.dli_fname, buffer);
+      if (resolved) {
+        int total_length = (int)strlen(resolved);


Should we prefer static_cast?

This suggestion applies in other places.

jjerphan · 2024-06-28T11:22:39Z

recipe/patches/0004-patch-zoneinfo_dir_override-to-point-to-our-tzdata.patch

+    // we're in $PREFIX/lib/libstdcxx.so
+#   define TO_PREFIX "/.."
+#   define PATH_SEP '/'
+    char buffer[PATH_MAX];


Out of curiosity, where is PATH_MAX defined?

I took this over from https://github.com/gpakosz/whereami, but some googling says its from <limits.h>.

recipe/patches/0004-patch-zoneinfo_dir_override-to-point-to-our-tzdata.patch

recipe/build.sh

h-vetinari · 2024-06-29T05:54:56Z

OK, this now passes some basic tests, though I'm sure the implementation can be improved (e.g. less allocations as @mbargull pointed out in #133). It's still using the -ldl debug work-around (when we should really be adding libdl to the link interface of libstdc++.so (equivalent of CMake's target_link_libraries), but as I noted in #133:

I think it should be something like AC_LIB_LINKFLAGS, but the makefile(s) in GCC are just indecipherable for me (though I really tried to find where the shared lib gets defined...). Any ideas how/where to do this?

CC @conda-forge/ctng-compilers

recipe/tests/tzdb.cpp

recipe/patches/0004-patch-zoneinfo_dir_override-to-point-to-our-tzdata.patch

…t to us

loading zoneinfo_dir_override as an extern function does not actually get the address of the function where it is defined (c.f. implementation ``` void* addr = __builtin_extract_return_addr(__builtin_return_address(0)); ``` ), but ends up returning the address of the file from where it is called. As such, in order to observe/test the zoneinfo_dir_override implementation, export libstdcxx's zoneinfo_file, which does exactly what's needed.

recipe/meta.yaml

h-vetinari · 2024-07-17T05:46:21Z

I haven't managed to get rid of -ldl being for linking to libstdc++.so, I'm either going to need a tip there how to add this to the link interface through autotools, or alternatively, we could just add -ldl to the default C++ flags in the activation feedstock.

@isuruf, this still fails when we compile something that needs libstdc++, because the -ldl is not automatically added. I don't know how to "bake this in"; unless you have an idea how to achieve this, I propose to add -ldl to the activation feedstock. WDYT?

isuruf · 2024-07-17T05:54:26Z

@isuruf, this still fails when we compile something that needs libstdc++, because the -ldl is not automatically added. I don't know how to "bake this in"; unless you have an idea how to achieve this, I propose to add -ldl to the activation feedstock. WDYT?

That's because libstdc++.so is not getting linked to libdl with my changes nor yours. Basically the patch you add doesn't work.

h-vetinari · 2024-07-17T06:01:20Z

Basically the patch you add doesn't work.

That's not a big surprise to me; it was an exploratory change, because I saw -lm in there, and hoped that it would similarly translate. I stared at the makefiles for a while, but I cannot even make out where the .so is defined, nor do I know for sure that AC_LIB_LINKFLAGS would be the right approach if I did find it.

recipe/patches/0006-add-ldl-to-libstdc___la_LDFLAGS.patch

h-vetinari · 2024-07-17T06:02:51Z

That's because libstdc++.so is not getting linked to libdl with my changes nor yours.

Though it is getting linked successfully while building with that change; before this patch, I needed to explicitly add -ldl in build.sh (see c01b031); after the patch that was not necessary anymore. However, that linkage information doesn't persist until after the build, when we're trying to link the test code with libstdc++

recipe/patches/0006-add-ldl-to-libstdc___la_LDFLAGS.patch

h-vetinari · 2024-07-17T06:07:12Z

If I do patch the Makefiles again, is it important for you to use the -Bstatic -ldl -Bdynamic -Wl,--exclude-libs,libdl.a stuff, or was that just exploratory as well?

mbargull · 2024-07-17T06:17:33Z

If I do patch the Makefiles again, is it important for you to use the -Bstatic -ldl -Bdynamic -Wl,--exclude-libs,libdl.a stuff, or was that just exploratory as well?

I haven't yet thought about the static linking case; I'll leave this one for @isuruf since he is more of an expert than me on it ;).

isuruf · 2024-07-17T14:38:00Z

Yes, we should link in libdl.a statically

….{am,in}`

conda-forge-webservices · 2024-07-25T07:45:07Z

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

This reverts commit 4b7df1f.

h-vetinari · 2024-07-25T12:09:13Z

This should be good now. Thanks for your help @isuruf & @mbargull!

h-vetinari · 2024-07-30T23:41:38Z

Gentle ping @conda-forge/ctng-compilers

h-vetinari · 2024-08-01T22:29:16Z

All good now?

isuruf · 2024-08-05T12:41:08Z

recipe/patches/0003-patch-zoneinfo_dir_override-to-point-to-our-tzdata.patch

+-#ifdef _AIX
+-  // Cannot override weak symbols on AIX.
+-  const char* (*zoneinfo_dir_override)() = nullptr;
+-#else


Let's keep this block or at least #error so that we don't trip if we build this.

Hmm, probably not needed since we have tests.

isuruf · 2024-08-05T12:41:54Z

Thanks

h-vetinari requested review from beckermr, isuruf, timsnyder and xhochy as code owners June 28, 2024 02:31

h-vetinari added 2 commits June 28, 2024 19:03

Revert "remove tzdb integration"

4ce469d

This reverts commit 2c40af6.

DEBUG: skip GCC != 14

4b7df1f

h-vetinari force-pushed the tzdb branch from 281a7fd to 4b7df1f Compare June 28, 2024 08:03

jjerphan suggested changes Jun 28, 2024

View reviewed changes

h-vetinari added 2 commits June 29, 2024 11:48

switch zoneinfo_dir_override from env vars to relative paths

72d69e4

link to libdl on linux

0b4eef7

h-vetinari force-pushed the tzdb branch 2 times, most recently from d5397db to cc5880e Compare June 29, 2024 01:02

DEBUG: add -ldl directly to CXX invocation

709153f

h-vetinari force-pushed the tzdb branch from cc5880e to 709153f Compare June 29, 2024 02:02

h-vetinari commented Jun 29, 2024

View reviewed changes

recipe/tests/tzdb.cpp Outdated Show resolved Hide resolved

h-vetinari mentioned this pull request Jun 29, 2024

libcxx v18.1.8 conda-forge/libcxx-feedstock#155

Merged

8 tasks

print the tzdb path we're using in the test

7807b91

h-vetinari force-pushed the tzdb branch 7 times, most recently from eebb928 to 0ed42a0 Compare July 1, 2024 06:02

jjerphan reviewed Jul 1, 2024

View reviewed changes

recipe/patches/0004-patch-zoneinfo_dir_override-to-point-to-our-tzdata.patch Outdated Show resolved Hide resolved

h-vetinari added 2 commits July 15, 2024 17:42

make tz_dir static; remove distracting #ifdef _AIX that's irrelevan…

2db0526

…t to us

h-vetinari force-pushed the tzdb branch from ab03fce to a3e796f Compare July 15, 2024 06:51

isuruf added 2 commits July 16, 2024 23:23

Use strace for a test

75648a1

'rerender' manually

7f897cf

isuruf reviewed Jul 17, 2024

View reviewed changes

recipe/meta.yaml Show resolved Hide resolved

mbargull reviewed Jul 17, 2024

View reviewed changes

recipe/patches/0006-add-ldl-to-libstdc___la_LDFLAGS.patch Outdated Show resolved Hide resolved

isuruf reviewed Jul 17, 2024

View reviewed changes

recipe/patches/0006-add-ldl-to-libstdc___la_LDFLAGS.patch Outdated Show resolved Hide resolved

no zoneinfo_dir_override in static builds; -ldl in both `Makefile…

1e6b3b9

….{am,in}`

h-vetinari added 3 commits July 25, 2024 20:17

don't move zoneinfo_file into __gnu_cxx namespace

c6214c7

re-order patches to make platform-distinction easier

1484ff4

Revert "DEBUG: skip GCC != 14"

730568f

This reverts commit 4b7df1f.

h-vetinari force-pushed the tzdb branch from 8a206fe to 730568f Compare July 25, 2024 11:57

Check override still works

a6d52a9

h-vetinari mentioned this pull request Jul 31, 2024

[rc] install C++20 modules; enable tzdb integration conda-forge/libcxx-feedstock#178

Merged

isuruf reviewed Aug 5, 2024

View reviewed changes

isuruf merged commit 2cdc2b9 into conda-forge:main Aug 5, 2024
64 checks passed

h-vetinari deleted the tzdb branch August 5, 2024 18:26

This was referenced Aug 6, 2024

Drop outdated sysroot #147

Open

conda-forge support for C++20 <chrono> man-group/sparrow#173

Open

h-vetinari mentioned this pull request Aug 21, 2024

tzinfo patch broken on windows #150

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tzdb integration #142

tzdb integration #142

h-vetinari commented Jun 28, 2024

conda-forge-webservices bot commented Jun 28, 2024

jjerphan left a comment

jjerphan Jun 28, 2024

h-vetinari Jun 29, 2024

jjerphan Jun 28, 2024 •

edited

Loading

jjerphan Jun 28, 2024

h-vetinari Jun 28, 2024

h-vetinari commented Jun 29, 2024

h-vetinari commented Jul 17, 2024

isuruf commented Jul 17, 2024

h-vetinari commented Jul 17, 2024

h-vetinari commented Jul 17, 2024 •

edited

Loading

h-vetinari commented Jul 17, 2024

mbargull commented Jul 17, 2024

isuruf commented Jul 17, 2024

conda-forge-webservices bot commented Jul 25, 2024

h-vetinari commented Jul 25, 2024

h-vetinari commented Jul 30, 2024

h-vetinari commented Aug 1, 2024

isuruf Aug 5, 2024

isuruf Aug 5, 2024

isuruf commented Aug 5, 2024

tzdb integration #142

tzdb integration #142

Conversation

h-vetinari commented Jun 28, 2024

conda-forge-webservices bot commented Jun 28, 2024

jjerphan left a comment

Choose a reason for hiding this comment

jjerphan Jun 28, 2024

Choose a reason for hiding this comment

h-vetinari Jun 29, 2024

Choose a reason for hiding this comment

jjerphan Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

jjerphan Jun 28, 2024

Choose a reason for hiding this comment

h-vetinari Jun 28, 2024

Choose a reason for hiding this comment

h-vetinari commented Jun 29, 2024

h-vetinari commented Jul 17, 2024

isuruf commented Jul 17, 2024

h-vetinari commented Jul 17, 2024

h-vetinari commented Jul 17, 2024 • edited Loading

h-vetinari commented Jul 17, 2024

mbargull commented Jul 17, 2024

isuruf commented Jul 17, 2024

conda-forge-webservices bot commented Jul 25, 2024

h-vetinari commented Jul 25, 2024

h-vetinari commented Jul 30, 2024

h-vetinari commented Aug 1, 2024

isuruf Aug 5, 2024

Choose a reason for hiding this comment

isuruf Aug 5, 2024

Choose a reason for hiding this comment

isuruf commented Aug 5, 2024

jjerphan Jun 28, 2024 •

edited

Loading

h-vetinari commented Jul 17, 2024 •

edited

Loading