felt / tippecanoe

Build vector tilesets from large collections of GeoJSON features.
BSD 2-Clause "Simplified" License
956 stars 83 forks source link

Versions after 2.0.0 fail to compile in Alpine Linux #24

Closed JamesChevalier closed 2 years ago

JamesChevalier commented 2 years ago

Something about https://github.com/felt/tippecanoe/commit/35b576d0fdbb5062a6a0f861e7eba1a3fc3891a2 (v2.1.0) broke compilation within Alpine Linux. It's corresponding pull request is https://github.com/protomaps/tippecanoe/pull/10

This can be reproduced via these steps:

The error message it produces is:

In file included from ./flatbuffers/verifier.h:21,
                 from ./flatbuffers/buffer_ref.h:21,
                 from ./flatbuffers/flatbuffers.h:25,
                 from flatgeobuf/feature_generated.h:7,
                 from flatgeobuf.cpp:5:
./flatbuffers/util.h: In function 'void flatbuffers::strtoval_impl(int64_t*, const char*, char**, int)':
./flatbuffers/util.h:226:38: error: 'strtoll_l' was not declared in this scope; did you mean 'strtold_l'?
  226 |     #define __strtoll_impl(s, pe, b) strtoll_l(s, pe, b, ClassicLocale::Get())
      |                                      ^~~~~~~~~
./flatbuffers/util.h:244:12: note: in expansion of macro '__strtoll_impl'
  244 |     *val = __strtoll_impl(str, endptr, base);
      |            ^~~~~~~~~~~~~~
./flatbuffers/util.h: In function 'void flatbuffers::strtoval_impl(uint64_t*, const char*, char**, int)':
./flatbuffers/util.h:225:39: error: 'strtoull_l' was not declared in this scope; did you mean 'strtoull'?
  225 |     #define __strtoull_impl(s, pe, b) strtoull_l(s, pe, b, ClassicLocale::Get())
      |                                       ^~~~~~~~~~
./flatbuffers/util.h:249:10: note: in expansion of macro '__strtoull_impl'
  249 |   *val = __strtoull_impl(str, endptr, base);
      |          ^~~~~~~~~~~~~~~
make: *** [Makefile:74: flatgeobuf.o] Error 1

(I noticed mentions of sh vs bash in https://github.com/felt/tippecanoe/issues/18 and tested this via bash with the same result)

I believe this is related to https://github.com/google/flatbuffers/issues/7265 which they resolved by moving #include "flatbuffers/util.h" from include/flatbuffers/verifier.h to src/binary_annotator.h (which doesn't exist in this repository) https://github.com/google/flatbuffers/pull/7266/files

Commenting out #include "flatbuffers/util.h" from flatbuffers/verifier.h resulted in a successful compilation.

I'm quite out of my depth at this point, so I don't want to waste anyone's time by submitting a pull request which removes that line if that's not the appropriate path forward. Another approach might be to replace the files with updated versions from https://github.com/google/flatbuffers/tree/master/include/flatbuffers 🤷

I'm happy to continue helping resolve this issue, but I think it's best if I get direction on which of the paths is most appropriate ... if either of them are 😅

bdon commented 2 years ago

@JamesChevalier yes, I think the best way is to replace the flatbuffers/ directory with updated versions. Do you want to open a PR?

At some point there seemed to be CI for building on different OS: https://github.com/felt/tippecanoe/blob/main/.travis.yml but I think travis is dead - would be nice to bring this back to catch these

JamesChevalier commented 2 years ago

Updating the files in flatbuffers went smoothly enough ( https://github.com/JamesChevalier/tippecanoe/commit/8e352d66c1083932820406cf77287e715de8e297 ), but I'm unsure how to generate the feature_generated.h and header_generated.h files where were included in the initial commit ( https://github.com/felt/tippecanoe/commit/a3dec5116dce33340dfb23a7669581625d0af8ee ).

https://github.com/google/flatbuffers/blob/master/docs/source/FlatBuffers.md includes:

Use flatc (the FlatBuffer compiler) to generate a C++ header (or Java/Kotlin/C#/Go/Python.. classes) with helper classes to access and construct serialized data. This header (say mydata_generated.h) only depends on flatbuffers.h, which defines the core functionality.

and https://github.com/google/flatbuffers/blob/master/docs/source/Compiler.md includes some usage notes that suggest a command along the lines of flatc --cpp -o flatgeobuf -I flatbuffers but I'm completely stumped on which file(s) I'd be pointing to for it to generate feature_generated.h and header_generated.h.

If it's easy enough to explain next steps, I'm happy to continue. If it's easier for you/someone else to take over entirely, that's ok too.

bdon commented 2 years ago

You would need to use the schema definitions here: https://github.com/flatgeobuf/flatgeobuf/tree/master/src/fbs

In fact, we should probably bump the generated headers in that repo simultaneously with updating them in this one

JamesChevalier commented 2 years ago

Thanks for your help, @bdon ... I was able to submit https://github.com/felt/tippecanoe/pull/26 with detailed notes in each commit. Hopefully those notes are helpful in confirming that I've done everything correctly.

In fact, we should probably bump the generated headers in that repo simultaneously with updating them in this one

I'm unsure of what's required within flatgeobuf for that suggested step. Are you suggesting I update the package.json from 2.0.7 to 22.9.29? (https://github.com/flatgeobuf/flatgeobuf/blob/master/package.json#L59)

bdon commented 2 years ago

I would suggest opening a PR to replace *_generated.h in https://github.com/flatgeobuf/flatgeobuf/tree/master/src/cpp and verify any programs in there work as well

JamesChevalier commented 2 years ago

Fixed in https://github.com/felt/tippecanoe/pull/26 and https://github.com/felt/tippecanoe/pull/30