Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MAC OS issue with types during the make command #664

Closed
amin212121 opened this issue Jan 30, 2024 · 10 comments · Fixed by #670
Closed

MAC OS issue with types during the make command #664

amin212121 opened this issue Jan 30, 2024 · 10 comments · Fixed by #670

Comments

@amin212121
Copy link

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
Copy link
Owner

What version of macOS?

What processor (Intel or Apple Silicon)?

@systemed
Copy link
Owner

...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
Copy link
Author

What version of macOS?

What processor (Intel or Apple Silicon)?

Sonoma 14.1
Apple Silicon

@amin212121
Copy link
Author

...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
Copy link
Owner

systemed commented Feb 1, 2024

That's great to know - thanks.

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

@cldellow
Copy link
Contributor

cldellow commented Feb 1, 2024

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
Copy link
Owner

systemed commented Feb 1, 2024

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
Copy link
Contributor

cldellow commented Feb 2, 2024

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:

  • broken Windows build
  • when I broke --store (in this PR -- caught before merged)

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
Copy link
Contributor

cldellow commented Feb 4, 2024

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 LostRuins/koboldcpp#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 #664 (comment)

@cldellow
Copy link
Contributor

cldellow commented Feb 7, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants