Implement for Haiku

[Kijewski]

Tested on Haiku R1/beta3.

The stress-test takes 1m43s. That's quite terrible.

[lopopolo]

Is binding to the haiku C++ API with a small native extension compiled by cc in build.rs an option here? This looks like it would be a lot simpler and less error prone since it is one API call.

https://www.haiku-os.org/docs/api/classBTimeZone.html

[Kijewski]

Sure! Do you have experience how to work with cxx?

BLocaleRoster::GetDefaultTimeZone() + BTimeZone::ID() + BString::CopyInto(&buf, 0, i32::MAX) should do exactly what we need.

[lopopolo]

I don't have experience with ccx to call this directly in Rust, but a tiny .cpp file with a C ABI export and one header can be made pretty usable with the cc crate and bindgen (or even a hand rolled single extern declaration) in a build.rs script.

I'd probably prefer to not add a dep on bindgen because it is fairly heavy but it also has a CLI which we could use to generate bindings from a C++ header and check in, similar to the approach you use for the tzdb perfect hash function.

[Kijewski]

It works. The stress-test now executes in 19 seconds.

Well, at least on an x86_64 system it works. But even if it only works for 64bit systems, I guess that's good enough.

[lopopolo]

This is looking great!

If you'd like to add clang-format to CI, we can use: https://github.com/artichoke/clang-format. I use it in Artichoke for C code. I recently added C++ support to the file scanner.

I tried it out with a simple .clang-format config and came up with this:

commit 20fa465a054534b98955a721ad5ba7dad03e851a (HEAD -> pr-haiku)
Author: Ryan Lopopolo <rjl@hyperbo.la>
Date:   Wed Sep 28 19:41:25 2022 -0700

    Add clang-format config

diff --git a/.clang-format b/.clang-format
new file mode 100644
index 0000000..444c17f
--- /dev/null
+++ b/.clang-format
@@ -0,0 +1,9 @@
+---
+BasedOnStyle: Google
+IndentWidth: 4
+ColumnLimit: 100
+---
+Language: Cpp
+SortIncludes: false
+TabWidth: 4
+UseTab: Never
diff --git a/include/impl_haiku.h b/include/impl_haiku.h
index cd50fe8..2d9c362 100644
--- a/include/impl_haiku.h
+++ b/include/impl_haiku.h
@@ -5,5 +5,5 @@
 #include <cstddef>

 namespace tz_haiku {
-    size_t get_tz(uint8_t *buf, size_t buf_len);
+size_t get_tz(uint8_t *buf, size_t buf_len);
 }

A GitHub Actions workflow to check this in CI looks like:

  c:
    name: Lint and format C
    runs-on: ubuntu-latest
    steps:
      - name: Checkout repository
        uses: actions/checkout@v3

      - name: Setup Node.js runtime
        uses: actions/setup-node@v3
        with:
          node-version: 16

      - name: Install npm
        run: npm i -f -g npm@8.16.0

      - name: Lint and check formatting with clang-format
        run: npx github:artichoke/clang-format --check

[lopopolo]

I left a suggested change to make the Rust side of things a bit easier to read and I was hoping we could add some comments in a couple of places on the C++ side of things.

Thanks for digging into cxx and the Haiku C++ docs. I like this a lot better than the initial pass at this PR.

[Kijewski]

I don't understand how this change could break the wasm implementation?? https://github.com/strawlab/iana-time-zone/actions/runs/3148078221/jobs/5118236934

[lopopolo]

I'm not sure either, but I bumped the minimum version of js-sys which seems to have fixed it for me locally

[lopopolo]

This is looking good! One more question on the C++ side.

[lopopolo]

@astraw this looks good to me. Do you want to take a quick look before we start the 7 day clock on this unsafe-proposed PR?

[astraw]

Thanks.

However, I think a build.rs on all platforms is a heavy price to pay to support a single platform, namely haiku. Can we move the haiku specific build.rs and cxx stuff to a separate crate (in the same git repo) so that build.rs is isolated to that platform?

[Kijewski]

build.rs's main function is empty for other OSes. cxx is only included for that one OS, because everything is wrapped in

    #[cfg(target_os = "haiku")]
    let _: () = {
        ....
    }

I don't think that having an empty build script would make a noticeably difference.

[astraw]

The concern is that the build script must be executed (not what it executes per se). I'm not so much worried about execution time etc but rather about cross compilation or using other toolchains etc.

[Kijewski]

Cross compilation is not affected, because the build script is always only executed on the host. At least I don't see how it could be affected right now.

E.g. if your build.rs to read /etc/hostname that will work on a linux host even when cross compiling to windows. But it will fail on a windows host, even when cross compiling to linux.

[astraw]

Yes, a build.rs file triggers building and execution of code on the host during cross compilation. My point is that this brings complexity for no gain on all platforms except Haiku. To me, the alternative is relatively painless and only paid by people building for Haiku.

[Kijewski]

Yeah, also this way users of other OS don't need to wonder why iana-time-zone executes a build script all of a sudden. I split off the code into its own crate.

[astraw]

Apart from the one trivial change, this looks good to me.

Thanks also for breaking this off into a separate crate. I think it is better that way.

[astraw]

We have 2 approvals now, so according to our Unsafe-Proposed policy (#64), let's let this sit until 8 October and then merge.

[astraw]

@Kijewski thanks for the work on this, including shifting most of it to its own crate.