-
Notifications
You must be signed in to change notification settings - Fork 8
Use Vcpkg manifest mode and add arm64 mac support #29
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
base: master
Are you sure you want to change the base?
Conversation
@@ -0,0 +1,28 @@ | |||
vcpkg_from_github( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One day will test if this is even needed for mac
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the patch just makes it so that MyGUI gets built as a regular library in a regular directory rather than a framework. In general, we want to upstream as many of our ports to mainline vcpkg (it's just openmw-osg being an application-specific fork that means it survived the last round of upstreaming), so if not being a framework is a good thing, we'll probably want to upstream this eventually.
8d4e1d3
to
539d460
Compare
Ended up adding mac CI |
853dfd7
to
a1badaa
Compare
b13669f
to
d0ada23
Compare
Quick update: I'm working on testing it with the CMAKE toolchain way rather than what I was doing. I'm also realizing I can have this include icu + qt, which reduces the need heavily for brew installed packages on the OpenMW side, so I'm seeing if that can work. |
Qt is huge and slow to build, and we don't get any particular advantage from having it in a precompiled deps package we maintain when upstream provides a perfectly good precompiled distribution and we're never going to want to patch it. For Windows, we use |
Updates:
|
4fa4200
to
6bb0105
Compare
I'd like to get this merged in some capacity so we have manifest mode in place so other platforms can mess with it. I can split out just to do the manifest stuff if preferred to that end. I mean, even if we merge this, we don't need to have Mac use it yet if there are kinks... |
There's a decent chance that that error would happen with the existing deps if they were to have assertions enabled, which I assume they don't. I'm not seeing a separate debug and release lib for Bullet in the archive, and I'm pretty sure it defaults to only enabling assertions in debug. Vcpkg, however, will give you both, so if you've built the navmesh tool in the Debug configuration, it'll link with a debug version of Bullet, and you'll see assertions that wouldn't fire before. |
script: | | ||
core.exportVariable('ACTIONS_CACHE_URL', process.env.ACTIONS_CACHE_URL || ''); | ||
core.exportVariable('ACTIONS_RUNTIME_TOKEN', process.env.ACTIONS_RUNTIME_TOKEN || ''); | ||
- run: brew install autoconf autoconf-archive automake |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a review comment, just a note so I'll see it if this line looks weird to me later.
Unlike on Windows, where vcpkg will install its own local isolated versions of tools like these, it explicitly recommends you install these yourself via Homebrew for MacOS.
Hmm. This makes sense. I'll rebuild in release mode and see if the issue persists. If it does NOT persist (i.e. what you said is the case), that would mean we must always build MacOS in release or else things would break that wouldn't before (i.e. introduce functional regression). Or we fix the underlying problem. I don't know quite why we wouldn't build in release for CI builds by default, esp. if they are the ones used in RC. I don't have any opinion on the current situation vs. what it should be, just saying that it would be a change in build procedure (whether it should happen anyway IDK). |
We build in |
We'll probably want to upstream the |
Building in release did fix the bullet navmesh issue. However, game doesn't launch due to libluajit issue, which is probably related to luajit instability on particular arches, I'm guessing. I'll have to test without using luajit.
|
RelWithDebInfo build: launches game. Yay. Navmesh tool now works. I wonder if it was something busted on my end/temporarily messed up 👍🏻 👍🏻 |
Navmesh doesn't work with my mod list, however, same error as before. I thought I was testing with clean list before, but maybe not. It errors out specifically on a certain cell, I'm guessing a Vivec one since it was in the middle of it. |
Game does eventually crash in Vivec when flying around. I suppose this would be some engine <> mod issue. |
I removed Baar Dau Ministry of Truth mod and it works. 🤷🏻 |
It does not completely work, it proceeded all the way to the end, bypassing what failed before and then crashed:
Which...is weird. But it still seems like this would be a mod/content <> engine issue somehow. |
Generally, if a disabled assertion would have failed, it's entirely normal if the application crashes. We only normally find out about failing assertions when someone's trying to debug something else that's too confusing with RelWithDebInfo and they try a debug build and find out debug builds have been broken for months. If there's a bug that would have triggered an assertion to fail, we usually find out because a user reports a problem with the same root cause and then we dig through a crash dump etc. rather than getting a clear error message from the assertion. |
So I've been building with RelWithDebInfo (which seems to be the standard across the board, the default for mac) except for that Release one time to test. Could it be that the current Mac deps (with no debug releases) were 'hiding' such errors, like you said, even in RelWithDebInfo mode? Like I've been building in RelWithDebInfo and the CI builds should be that too. So the mode shouldn't have changed. Only the deps. But to update on the navmesh issue, I identified another mod that was causing problems, removed it, and it finished off. I could raise an issue on OpenMW proper, like I don't think it makes sense that the tool is failing for certain valid content (i.e. mods)....why it is failing now and not before is confusing, though. I did rebase, though, but I don't think that's relevant. |
If you're using a slightly different commit of Bullet, then the issue caught by the assertion might change from asymptomatic/subtly symptomatic into a full-on crash. As a sanity check, you could temporarily try with the exact same Bullet commit as the previous Mac deps used. |
note: bullet3 from vcpkg is 3.25 whereas what openmw extern/old deps use is 3.17. |
Yeah, that's a big version jump. There's no reason to expect situations where assertions happen to have remotely the same symptoms when they're disabled over that big a range. If you can come up with a minimal repro, it's really easy to try a debug build on Windows, so we should be able to verify that the assertion failure is cross-platform, fix it, and then verify the fix without too much trouble and then test again on MacOS. |
@AnyOldName3 Okay, easy (I can x-post to OpenMW issue if desired). Have OAAB/Tamriel data installed and install https://www.nexusmods.com/morrowind/mods/50877?tab=posts See example below
Run navmesh tool. |
Yeah, it'll probably want a proper issue report on the main tracker. |
I attempted to downgrade to bullet 3.17 and still ran into the same error, which is...frustrating. I'll raise an issue. |
How should I proceed here? Is there anything to be done for this PR? |
I was hoping someone would have figured out if the Bullet issue was happening on other platforms and/or versions by now, but no one's got around to it yet. The big warning-fixing MR is sapping a lot of the attention of a few of us, for example. |
I've been vaguely watching that MR—I'm wondering if it could fix the bullet issue or make the behavior of the issue more consistent across dep source. |
No Mac CI job yet, will raise in subsequent PR.Mac dependencies do build a functioning OpenMW that runs the game.
MacOS specifics taken from https://gitlab.com/OpenMW/openmw-dep
New Notes:
This raises the target to MacOS 15
Also the 7z contains more than just what we'd need—I'm guessing windows is the same way with the raw export. One day we could maybe trim what we store.