-
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
Flex: Conan v2 support #14013
Flex: Conan v2 support #14013
Conversation
- Doesn't work well with the new Conan v2 tools
I detected other pull requests that are modifying flex/all recipe:
This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there. |
The
CMake has a built in module for finding Any advice on what to do for this? I suspect that |
This comment has been minimized.
This comment has been minimized.
- Doesn't look like there's a replacement for tools.which() in Conan
Related issue about |
This comment has been minimized.
This comment has been minimized.
Hooks produced the following warnings for commit b40b1bfflex/2.6.4
|
# Find FLEX before `conanbuildinfo.cmake` because that file will let `find_program` | ||
# look for executables in host packages (let's hope conan 2.0 fixes this) |
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.
https://cmake.org/cmake/help/latest/module/FindFLEX.html 🤔 Usually it's just variables so it's say #13511 (comment)
If this was an output of flex's CMake install we could use build_modules
but I am not sure if thats the case?
Just a note this recipe is missing the cmake target definitions so you'll want to add those
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.
You should be able to inject the necessary variables in the CMakeToolchain via conf
. Generate a small CMake script that sets the necessary Flex variables and add it to tools.cmake.cmaketoolchain:user_toolchain
so that anything that tool_requires
Flex will inherit those CMake variables. You probably will only need to set FLEX_EXECUTABLE
, FLEX_LIBRARIES
, and FLEX_INCLUDE_DIRS
. Then maybe see what happens if you rename Conan's generated Config module to something like _Flex
so that nothing will actually use it.
Alternatively, if you can add a build module that includes CMake's FindFlex
module, that may also work. You'd probably have to set the same variables before including it.
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.
Thanks to @prince-chrismc and @jwillikers for the insights. The suggestion to set up an environment variable gave me an idea. I went and looked at the code for FindFLEX
. It's internally using find_program
, find_library
and find_path
, all of which will use FLEX_ROOT
as the first place to search, for CMake >= 3.12. See also the doc for find_library
.
Turns out the following works, at least in my local testing:
- Point
FLEX_ROOT
at the package. - Keep Conan from replacing CMake's built-in
FindFLEX
. For CMakeDeps, have the recipe produce nothing for CMake, and for the old generators, make the package with a different name.
- Also remove commented code
- Based on the imports from conan.tools
Replacing the internal find module causes CMake to not supply the flex_target macro. - For CMakeDeps, don't export a dependency at all. - For cmake_find_package and cmake_find_package_multi, name the package _FLEX to not conflict with the built-in one. - Set FLEX_ROOT to help CMake find the flex binaries, libraries, and headers in the Conan package. This works with CMake 3.12 or newer.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I need some help with the CI. It's failing the arm64 build. I'm at a loss because I can build, and even cross-build, on an M1 machine locally. The root cause seems to be the architecture of the library is wrong, in this log.
I got onto a MacBook Air M1 and it builds fine. I even tried to invoke the cross-building that seems to be going on in CI, and that also worked.
|
I'm kind of dead in the water on this one. I tried to reproduce the problem locally again today, and as mentioned above, I can't produce the build problem locally on an M1 machine. |
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.
@datalogics-kam Thanks for working so hard on this. It's a really big update. I'm not exactly sure the root cause of your error, but I'm pretty certain that the test_package is attempting to link with the Flex library from a different architecture. I recommend seeing if it's possible to update things to align as much as possible with the Autootools Template Package. That might help solve the problem, or at least reduce the number of potential problems.
recipes/flex/all/conanfile.py
Outdated
autotools.configure(args=configure_args, configure_dir=self._source_subfolder) | ||
return autotools | ||
tc = VirtualBuildEnv(self) | ||
tc.generate(scope="build") |
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.
You can omit scope="build"
here as it is the default.
recipes/flex/all/conanfile.py
Outdated
f"--enable-shared={yes_no(self.options.shared)}", | ||
f"--enable-static={yes_no(not self.options.shared)}", |
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.
AutotoolsToolchain should set these two variables automatically: https://docs.conan.io/en/latest/reference/conanfile/tools/gnu/autotoolstoolchain.html#attributes
Co-authored-by: Jordan Williams <jordan@jwillikers.com>
This comment has been minimized.
This comment has been minimized.
- Use tool_requires instead of build_requires - Don't define --enable-shared or --disable-shared; they're automatic.
- Idea copied from the CMake recipe
- Defining C just caused a build error; there's C++ code in the CMakeLists.txt. - While checking the doc for project(), the default for LANGUAGES is C and CXX anyway, which is what is needed. https://cmake.org/cmake/help/v3.25/command/project.html#options
I did a bit more cleaning up. The root of the problem seems to be the failure when cross-building with x86_64 build environment and armv8 host, which is what the M1 CI builder is doing.
If there's another way to get a CMake variable defined, or a way to extend or wrap a built-in module, I'd be interested in hearing about it. |
Conan v1 pipelineFailure in build 7 (
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. |
@datalogics-kam I've had to do something similar with Meson / PkgConfigDeps for the Wayland package, see here. You might be able to make this work by using the CMakeDeps features build_context_activated, build-context-suffix, and build_context_build_modules in the Ideally, it would be possible to more cleanly separate the CMake modules generated by Conan into different directories and point CMake to the correct ones based on the build / host contexts. This wouldn't require renaming the CMake modules for the build context, which requires changes to the underlying build system. There's an open an issue regarding this behavior: conan-io/conan#12342. |
Specify library name and version: flex/2.4.1
Add v2 support to flex.