-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
openfst: also permit building on Macos #23592
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
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.
Hi! Locally I'm having problems building this with any of enable_compact_fsts, enable_ngram_fsts enable_const_fsts
set to true. Is this expected? Or should it be validated out?
Interesting -- I seem to have no issues building locally even when I enable all the non-default-enabled flags:
This is on a M3 Max MacBook Pro with Sonama 14.4.1 and conan version:
I'm not super familiar with conan overall (this is my first time using it!) so I might not be great at debugging this, but can you say more about what sort of failure you're observing/your build environment? |
Thanks for double checking, let me get you some build logs later today! ❤️ |
The compilation logs are extremely verbose, the failing bit is in the test package though:
|
Aha! I was building without I might need some more time to dig into exactly what's going awry and if there's a way to make it work; if I get stuck, would it be okay for us to only allow |
Okay, so the final linker error we get is something like (whitespace/newlines added for readability):
Looking in more details at some of those problematic .so files:
we find that the filetype is 8, which is It's weird to me that we see both things like conan-center-index/recipes/openfst/all/conanfile.py Lines 200 to 203 in 9399ff6
I find if I comment those 4 lines out, I am able to build successfully with It's not clear to me why those four lines exist at all; based on the logic it seems like they would always be redundant with the lines at conan-center-index/recipes/openfst/all/conanfile.py Lines 183 to 186 in 9399ff6
|
Yes! That would would for now, thanks a lot for taking a look into it :) |
For what it's worth, in the absence of any explanation to the contrary nor any comments I can find, I'm inclined to believe the lines at:
are either buggy or at least vestigial and should be removed. For example, when I try to build with all the flags enabled on x64 Linux with:
then Since this seems to improve things for both x64 Linux and arm64 macOS, I'm inclined to just amend the patch to remove this (and the now-unused helper property functions). |
On macOS the files referenced have the wrong type (MH_BUNDLE vs MH_OBJECT or MH_DYLIB) and so the linker fails. On Linux, the additional files referenced cause dynamic link errors when trying to execute the test_package when building with -o shared=True and -o enable_const_fsts=True because the dynamic linker cannot find them. Neither failure mode seems to persist if we simply use only the full named library artifacts, so in the absence of a comment explaining the logic or any reference in the git history, I'm inclined to simply remove the additional .so references.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ✔️All green in build 4 (
Conan v2 pipeline ❌
The v2 pipeline failed. Please, review the errors and note this is required for pull requests to be merged. In case this recipe is still not ported to Conan 2.x, please, ping See details:Failure in build 4 (
Note: To save resources, CI tries to finish as soon as an error is found. For this reason you might find that not all the references have been launched or not all the configurations for a given reference. Also, take into account that we cannot guarantee the order of execution as it depends on CI workload and workers availability. |
Specify library name and version: openfst/1.8.2
Nutrimatic makes use of openfst as built by conan; I wanted to build it for macOS but this recipe errored out because its
validate
rejects running on anything besides "Linux" and "FreeBSD". Empirically, openfst does seem to build and work on macOS successfully with conan 2.1.0 with this patch applied, so this restriction can likely be loosened.I've not tried the local configuration with the conan-center hook because I'm using conan 2, and the hooks repository says