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

Use Homebrew's std_cmake_args #6

Merged
merged 1 commit into from Jan 10, 2021
Merged

Conversation

jlaxson
Copy link
Contributor

@jlaxson jlaxson commented Jan 5, 2021

Among several things, this explicitly passes the correct Xcode sysroot to cmake and on to clang. Without it, can spuriously get failure to find system symbols similar to these issues: https://stackoverflow.com/questions/58628377/catalina-c-using-cmath-headers-yield-error-no-member-named-signbit-in-th

I don't know the rhyme or reason to why it works sometimes (e.g. magnum from this repo worked for me, but magnum-plugins didn't).

@mosra
Copy link
Owner

mosra commented Jan 5, 2021

Ohhhh so that's the solution, thanks! ❤️

And I can tell you why the problem happens -- I did a full investigation not so long ago: https://doc.magnum.graphics/magnum/platforms-macos.html#platforms-macos-troubleshooting-build-mysteriously-fails

Hum and I see some new issues related to SPIRV-Tools, gotta investigate (if you know what changed in that package, please share!).

@mosra mosra added this to the 2020.0b milestone Jan 5, 2021
@jlaxson
Copy link
Contributor Author

jlaxson commented Jan 5, 2021

I'm not sure what the difference is, I'm on Mojave same as the circle image and it is working fine for me from a fresh reinstall of spirv-tools and magnum-plugins. But, when I look at the cmake code it installs, nothing actually does add_library(SPIRV-Tools), only SPIRV-Tools-shared and similar. I think it's a bug in spirv-tools - they have that target as an alias internally, and try to export it, but cmake doesn't export/install aliases. Still no clue why it works for me.

I do notice that the brew tap in the circle config doesn't actually check out the branch under test, not sure how intentional that was. I think you'd want a manual fetch/reset to the appropriate ref, or use the circle checkout command explicitly rather than homebrew's command.

@jlaxson
Copy link
Contributor Author

jlaxson commented Jan 5, 2021

Also there is a note at the top of the install step re: unshallowing the homebrew clones, don't know if that means it's getting a different version of the formula than I have

@mosra
Copy link
Owner

mosra commented Jan 5, 2021

But, when I look at the cmake code it installs, nothing actually does add_library(SPIRV-Tools), only SPIRV-Tools-shared and similar.

Now I remember -- that's it. I was aware of this change and just put it aside "for now". Seems like that commit got released, so I have to account for that in my Find module.

I do notice that the brew tap in the circle config doesn't actually check out the branch under test

Yes, good point -- is there a way to make homebrew use a certain branch of the tap? (I assumed there was no way and so I usually "tested" by pushing straight to master.)

@mosra mosra mentioned this pull request Jan 5, 2021
144 tasks
@jlaxson
Copy link
Contributor Author

jlaxson commented Jan 5, 2021

I think (think) you could replace run: brew tap mosra/magnum with

checkout:
  path: /usr/local/Homebrew/Taps/mosra/homebrew-magnum

Which will use circle's logic to get the right code and put it where homebrew is looking.

@mosra
Copy link
Owner

mosra commented Jan 5, 2021

Yes, but I'm afraid this kind of a hack might hide issues that would happen only with brew tap.

Or I guess I could brew tap on master and do this on other branches, that could be sufficiently bulletproof.

@mosra
Copy link
Owner

mosra commented Jan 9, 2021

Ok, the spirv-tools issue got fixed worked around in mosra/magnum-plugins@d8d016e (it's actually an issue with spirv-tools own cmake being totally broken, so I had to patch get_target_properties() to not die with a fatal error when that happens).

I restarted the PR build, but got greeted by

[ 15%] Linking CXX executable ../../Release/bin/magnum-bullet
Undefined symbols for architecture x86_64:
  "btDiscreteDynamicsWorld::stepSimulation(float, int, float)", referenced from:
      Magnum::Examples::BulletExample::drawEvent() in BulletExample.cpp.o

so I suppose that's another fresh exciting breakage in a 3rd party dependency. Investigating...

@mosra
Copy link
Owner

mosra commented Jan 9, 2021

Bullet package breakage fix submitted as Homebrew/homebrew-core#68634, with additional work done in mosra/magnum-integration@c9f9a0d & mosra/magnum-integration@8585e34 to turn linker errors into something more meaningful. Waiting until the package update happens, then will finally merge this.

@mosra mosra merged commit 56a33c1 into mosra:master Jan 10, 2021
1 of 2 checks passed
@mosra
Copy link
Owner

mosra commented Jan 10, 2021

Seems like it did the job, I don't need any workarounds to build magnum-plugins anymore. Magic.

In 4020b83 I made the change to check out a branch as you suggested and it works well. Thanks for that as well :)

@jlaxson
Copy link
Contributor Author

jlaxson commented Jan 10, 2021

# What kind of INSANE language is this?!

Story of my life

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

Successfully merging this pull request may close these issues.

None yet

2 participants