-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
flatbuffers: refactoring #9292
flatbuffers: refactoring #9292
Conversation
By building flatc always I cannot use this recipe anymore for platforms where flatc cannot be build. I'm not sure if there are still platforms that do not support flatc but do support flatbuffers. This is not such a weird situation, there are multiple platforms where the compiler/code generator is more restrictive that the produced code. This was the original reason for the weird option_from_context as it was the only way I could find to include flatc in the build context but not in the host context. |
This comment has been minimized.
This comment has been minimized.
Which platforms? If there are platforms not supporting flatc executable, I can keep |
This comment has been minimized.
This comment has been minimized.
QNX for example. Not sure about others. I don't think you can set the flatc option one way for the build context and a different for the host context. Or is this possible now? |
If I add flatc option again, it would be possible: conanfile.txt
EDIT: actually, What is the error on QNX? |
This comment has been minimized.
This comment has been minimized.
@jgsogo what is exactly the command executed in CI? This error should never happen with |
Hi! The error comes from the When building for Apple M1, the command is
And the packageID computed for each context are different:
Of course, the packageID for the build context is missing because it hasn't been built. When creating the package in the previous step we build the one for the host context. We cannot test in 1️⃣ It can succeed if the package for the build context has already been built by another thread and uploaded to the remote... typically, it will succeed if we retrigger the build one more time for the same recipe revision. This is probably the reason why other packages have succeeded. Having a look at the logs, they are using two different packageIDs:
and if they work, it is just because that packageID has been generated before by a different job. Of course we cannot rely on this behavior. |
btw, thanks a lot for taking care of this recipe, all those fixes were really needed 🎉 @mjvankampen , if |
Thanks for the detailed explanation. So what should I do here?
QNX is not referenced in default I guess something like this would be fine, but it's weird to reference an OS id not even supported in settings.yml config_options(self):
if str(self.settings.os).upper() == "QNX":
self.options.flatc = False |
Indeed, we can only test the build context if both profiles (host and build) are the same (same packageID). So far this only happens (atm in CCI) in scenarios other than cross-building ones. For the future, Conan should provide a way to indicate that the |
This comment has been minimized.
This comment has been minimized.
There is a serious issue in 2 profiles I think. Even with this trick, if someone has flatbuffers in requirements (for the library) and build requirement (for flatc), and flatc option is False in requirements only, it won't work because build requirement will inherit flatbuffers:flatc=False (and it should not obviously): conan-io/conan#10425 (comment) |
@mjvankampen would it be fine with 8b99a52? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
All green in build 4 ( |
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.
let's move this one forward @SpaceIm . there are serious issues with 2 profiles in v1, but nothing we can do about it in CCI right now :(
I don't think it should block your PR, as it's an improvement on its own.
Hi @SpaceIm , After reading this discussion loop, I still got following 2 questions.
Thanks. |
Have the same problem on linux as @qiao-tw when building onnxruntime/1.7.1 package with flatbuffers/1.12.0 what is the current solution for this? |
flatc
,flatbuffers
andoptions_from_context
options (there was an attempt in [flatbuffers] remove misleading option #4863). All these options were part of a counterintuitive mechanism preventing to build the lib in build context and the executable in host context.Instead:
flatc
is always built now, as well as flatbuffers lib (either static, shared or header only).flatbuffers::flatc
target (like upstream config file), and handle executable discovery from build context if cross-building with 2 profiles (it was managed by the weirdoptions_from_context
before), or host context if native build. It's a more elegant solution than Flatbuffers flatc integration #3345, and similar to the trick used inprotobuf
andgrpc
recipes.FindFlatBuffers.cmake
, while config file isFlatbuffersConfig.cmake
): see https://github.com/google/flatbuffers/tree/v2.0.0/CMakeBuildFlatBuffers.cmake
throughfind_package()
(iecmake_find_package
/cmake_find_package_multi
/CMakeDeps
). At some point it doesn't make sense and it's not worth the effort to support consumers trying to call these functions without callingfind_package(Flatbuffers)
before.It's worth noting that this module file & its functions are not part of upstream config file, only Find module file. I've added this file to both, but I don't recommend to rely on these functions.
/cc @jgsogo @mjvankampen
closes #5616
closes #8374
conan-center hook activated.