systemed / tilemaker

Make OpenStreetMap vector tiles without the stack
https://tilemaker.org/
Other
1.42k stars 228 forks source link

MAC OS issue with types during the make command #664

Closed amin212121 closed 6 months ago

amin212121 commented 6 months ago

I run make command on MAC OS but get the errors:

src/helpers.cpp:163:16: error: variable has incomplete type 'struct stat64' struct stat64 statBuf; ^ src/helpers.cpp:163:9: note: forward declaration of 'stat64' struct stat64 statBuf; ^ src/helpers.cpp:164:6: error: no viable conversion from 'stat64' to 'int' int rc = stat64(filename.c_str(), &statBuf); ^ ~~~~~~~~~~ 2 errors generated. make: *** [src/helpers.o] Error 1

image
systemed commented 6 months ago

What version of macOS?

What processor (Intel or Apple Silicon)?

systemed commented 6 months ago

...and I haven't tested it, but adding this near the top of src/helpers.cpp may work:

#if defined(__APPLE__)
#define stat64 stat
#endif
amin212121 commented 6 months ago

What version of macOS?

What processor (Intel or Apple Silicon)?

Sonoma 14.1 Apple Silicon

amin212121 commented 6 months ago

...and I haven't tested it, but adding this near the top of src/helpers.cpp may work:

#if defined(__APPLE__)
#define stat64 stat
#endif

yes it helps. Thx

systemed commented 6 months ago

That's great to know - thanks.

@cldellow Ok just to add that, do you think?

cldellow commented 6 months ago

I think so!

There's probably another solution, too. It looks like perhaps this is only for the Makefile build, not the CMake build, which makes me think we must have subtly different include directory orders or something?

Sort of related: would you be open to a PR to add the Makefile builds to the commit jobs? That would help catch these issues sooner. I'm not sure how supported they're intended to be. I use them myself, but that's mainly a path of least resistance thing.

systemed commented 6 months ago

Sort of related: would you be open to a PR to add the Makefile builds to the commit jobs?

Yep, that would be good!

Also tangentially related... I was thinking about how we check for failed Windows builds like #661. At present we try to generate an .mbtiles of Liechtenstein, but we don't actually check to see whether it's been successfully generated or not.

cldellow commented 6 months ago

Ah, I see all the PR builds eat errors with || true.

Two easy verifications we could add:

  1. Did the process exit with status code 0?
  2. Does the file have the expected # of tiles and bounding box (use sqlite3 or go-pmtiles to confirm)

If we did two generations (store/mbtiles, no store/pmtiles), it would have caught:

We could go further: dump a "fingerprint" of the output, and compare it against a gold standard. For the sake of discussion, this inscrutable bit of bash:

echo "select zoom_level || ' ' || tile_column || ' ' || tile_row || ' ' || hex(tile_data) from tiles order by zoom_level asc" |
  sqlite3 monaco.mbtiles  |
  while IFS=" " read -r z x y hex; do
    echo -n "$hex" | xxd -r -p > tile
    echo "z=$z x=$x y=$y"
    vt2geojson tile -x $x -y $y -z $z | jq --sort-keys -rc .features[] | jq -rc '. | {type: .geometry.type, properties}' | sort | uniq -c | sort -nk1 --reverse
  done

produces https://gist.github.com/cldellow/a8ed32a83190fb91ff74d0743b400d6e. I think if we compared PRs against that, it would likely have further caught:

A downside of that approach is that it would also act like a code change detector. For example, if we updated the OMT profile or changed simplification, it might generate a false positive. You'd need to re-run the command to produce a new gold standard. The file itself is also quite big -- 400kb -- repeatedly checking in revisions of such large files would bloat the git repo, so perhaps it ought only be done for a single tile, not all of the tiles.

If we had some fingerprint/canonicalization tool, we could do a separate verification: fingerprint the store/mbtiles output and the no-store/pmtiles output (including their geometries), and assert that they're the same. This would catch discrepancies between MBTiles/PMTiles and lazy/materialized geometries.

cldellow commented 6 months ago

I'm working on a PR to improve the build and did some exploring here. My theory that it was a Makefile vs CMake thing seems wrong -- the Makefile build passes in a Mac OS X action.

Another project hit this in https://github.com/LostRuins/koboldcpp/issues/201. The clang version cited there is the same clang version we use in GitHub actions, so it seems like Apple Silicon is to blame.

Oh, I see that GitHub released Apple Silicon support for open source projects two days ago: https://github.blog/changelog/2024-01-30-github-actions-introducing-the-new-m1-macos-runner-available-to-open-source/

I'll see if I can add them into the build and repro the issue, and then send a PR that makes the change you propose in https://github.com/systemed/tilemaker/issues/664#issuecomment-1917866605

cldellow commented 6 months ago

A fingerprinting approach / something that captures the # of features would also have caught the regression identified in #671, I think.