migrate CI to M1 #4500
migrate CI to M1 #4500
Conversation
|
oh nice, M1 cuts down the build time by half! |
conanfile.txt
Outdated
| @@ -1,22 +1,19 @@ | |||
| [requires] | |||
| boost/1.71.0 | |||
| boost/1.83.0 | |||
There was a problem hiding this comment.
boost 1.71 can't be used anymore on newer osx versions, it uses functions which were removed in c++17
conanfile.txt
Outdated
|
|
||
| [generators] | ||
| cmake_find_package | ||
| json | ||
|
|
||
| [options] | ||
| boost:header_only=True | ||
| boost:without_atomic=True |
There was a problem hiding this comment.
it wouldn't install boost without having these libraries as well. it's not compiling anything though, I guess it's just optional dependencies it now expects for some reason
|
gotta be fucking kidding me.. still the same bullshit error as on intel osx.. see here: #4253 (comment). at least those hours debugging it weren't for naught.. |
src/mjolnir/adminbuilder.cc
Outdated
| @@ -142,6 +142,7 @@ void buffer_polygon(const polygon_t& polygon, multipolygon_t& multipolygon) { | |||
| auto* outer_ring = geos_helper_t::from_striped_container(polygon.outer()); | |||
| std::vector<GEOSGeometry*> inner_rings; | |||
| inner_rings.reserve(polygon.inners().size()); | |||
| std::cout << polygon.inners().size() << std::endl; | |||
There was a problem hiding this comment.
this is crap of course, even though it makes the build pass! open for ideas what to do here.. smth that calls polygon.inners().size(), has no impact on anything and doesn't get optimized away
There was a problem hiding this comment.
fwiw, it doesn't help at all to check for its size for some reason, it'll still enter that condition.. kinda reminds me of python generators, which you can call only once. the second time the same instance is called, they're exhausted.
There was a problem hiding this comment.
could also make it a LOG_INFO and pretend it's useful info :D
There was a problem hiding this comment.
@ianthetechie can you do me a favor and try to run the „gurka_admin_uk_overrides“ (smth like that) with this branch, but without this log line? My M1 is still set up with Rosetta and I lack the patience to switch, my intel osx compiles fine, but CI keeps crapping out. On intel I was on clang 14. This seems like an outrageous bug to me😄if we can merge this, we can finally get rid of conan and have full support for vcpkg which would be great.
There was a problem hiding this comment.
Sure thing @nilsnolde 😂
By "set up with Rosetta" I assume you mean the thing in the docs about configuring your terminal and stuff with Rosetta and running builds there, right? If so then yeah, I ignored that :P
On to the test... it works for me in debug configuration with this log line removed. When I run in release mode, I get an error about mode_change_count in thor/astar_bss.cc being set but never referenced cuz I guess the assert macro is a no-op in release mode :P Removing that variable and references, it compiles and the gurka_admin_uk_override test passes.
There was a problem hiding this comment.
And yeah, we're running the same Xcode build (for another day or 2 at least; IIRC Xcode 15.2 just dropped and I'll upgrade soon but currently running 15.1).
There was a problem hiding this comment.
I'm also running Boost 1.83 FWIW. Here's my CMake output.
There was a problem hiding this comment.
So your env is as close as it can be to CI, yet it passes for you 🤯 I'd like to test out GHA, but we only get crap runners there.
There was a problem hiding this comment.
Hmm, yeah that's really strange. Mine succeeds on that particular test.
I can't actually complete the full test suite though. It craps out in the Python section, probably because I haven't configured my environment perfectly for it. (Worth noting btw that newer versions of python/pip will refuse to sudo install packages globally, so we'll probably need to change how that part of the process to use virtual environments etc.)
The only (highly unsatisfying) explanation that this suggests is something peculiar to running tests in parallel. This doesn't make a lot of sense to me, but I've experienced similar issues in the past (ex: #4048; I think this one may have resolved itself now).
There was a problem hiding this comment.
It craps out in the Python section
Yeah I can see in your CMake output that it can only find the Interpreter, for the bindings you'll need a dev installation (or point CMake to the right libs/dirs).
Worth noting btw that newer versions of python/pip will refuse to sudo install packages globally
We already took care of that:)
I'm very convinced this is a compiler bug, somehow only manifesting in circleci OSX machines :D In the admins we don't do any parallel stuff, we also don't depend on tiles/graphreader or so around that code. But 🤷 , I'm done with this crap for now really.
|
not bad, a full osx build in 16 mins! |
test/util_midgard.cc
Outdated
| @@ -416,7 +416,7 @@ TEST(UtilMidgard, TestTrimPolylineWithFloatGeoPoint) { | |||
| // Worst case is they may quantized at 1.69m intervals (for an epsilon change). | |||
| // https://stackoverflow.com/a/28420164 | |||
| // The length comparisons below do better than that, but not a lot. | |||
| constexpr double MAX_FLOAT_PRECISION = 0.05; // Should be good for 5cm at this lon/lat | |||
| constexpr double MAX_FLOAT_PRECISION = 0.06; // Should be good for 5cm at this lon/lat | |||
There was a problem hiding this comment.
didn't we JUST touch this somewhere? I'm so sure about it, but couldn't find the PR.. anyways, M1 exceeds the precision.
There was a problem hiding this comment.
Can confirm; I'm using this on my local working copy or else tests fail on Apple Silicon.
There was a problem hiding this comment.
I also found where I touched it last, in the other PR: https://github.com/valhalla/valhalla/pull/4253/files#r1442377392, where an even bigger precision tolerance is needed
|
Well finally the |
|
This is as mergeable as it'll get with circleci |
hoping this will not end in a shit show 🤞