-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
#3907: add component info in conanfile.py, add cmake helper module, e… #3908
#3907: add component info in conanfile.py, add cmake helper module, e… #3908
Conversation
…odule, export location of build arch flatcc exe when cross-compiling
This comment has been minimized.
This comment has been minimized.
recipes/flatcc/all/conanfile.py
Outdated
self.cpp_info.libs.append("flatcc%s" % debug_suffix) | ||
self.cpp_info.libs.append("flatccrt%s" % debug_suffix) | ||
self.cpp_info.components["flatcc_exe"].names["cmake_find_package"] = "flatcc_exe" | ||
self.cpp_info.components["flatcc_exe"].libs = ["flatcc%s" % debug_suffix] |
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.
If flatcc_exe
is the compiler binary, shouldn't .libs
be empty then?
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.
Yes, I will add an extra component for the library that the compiler binary links to.
@@ -0,0 +1,88 @@ | |||
|
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.
Can you please point me to the cmake script from the flatbuffers repo at https://github.com/dvidelabs/flatcc
containing these functions/macros?
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 wrote that cmake script because it simplifies using flatcc in cmake a lot. I am going to send it upstream as well.
Do you want me to add it to the flatcc package via the patches mechanism?
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.
Edit: PR for flatcc cmake module submitted: dvidelabs/flatcc#169
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.
Please do it as a patch, so when your pr gets accepted, this recipe can be adapted more easily.
In the mean time, maybe add a FIXME/TODO that this is experimental conan-only code?
…component, add cmake helper module via patch
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.
PR paused due to discussion and modifications of the upstream PR. |
@madebr With the recent changes in your flatcc cmake_dep branch, should I add two components in the flatcc Conan recipe:
That would implicate that I have to add the compiler lib to component cli. |
Indeed, 2 components BTW, the installed cmake scripts are perfectly relocatable. |
Thanks. |
Indeed, and keep the |
…madebr's cmake_dep branch until merged upstream. Note this is work-in-progress, not to be merged until new flatcc release is available
This comment has been minimized.
This comment has been minimized.
I have modified the flatcc Conan recipe to the modified flatcc cmake. Temporarily it fetches your cmake_dep branch. This works fine with command
the environment variable FLATCC_CLI_EXE is not set and the build fails :-( |
This comment has been minimized.
This comment has been minimized.
@@ -99,12 +109,36 @@ def package(self): | |||
os.path.join(self.package_folder, "bin", "flatcc")) | |||
# Copy license file | |||
self.copy("LICENSE", dst="licenses", src=self._source_subfolder) | |||
# Remove cmake config files | |||
tools.rmdir(os.path.join(self.package_folder, "lib", "cmake", "flatcc")) |
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.
Is flatcc
present at install time?
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.
Nevermind, it is!!!! Doh.
@@ -99,12 +109,36 @@ def package(self): | |||
os.path.join(self.package_folder, "bin", "flatcc")) | |||
# Copy license file | |||
self.copy("LICENSE", dst="licenses", src=self._source_subfolder) | |||
# Remove cmake config files |
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.
Alternatively, move the files you want to keep to some temporary folder, remove os.path.join(self.package_folder("lib", "cmake"))
and re-create a cmake
folder
recipes/flatcc/all/conanfile.py
Outdated
genSourcesMod = os.path.join(self.package_folder, "lib", "cmake", "flatcccli", "FlatccGenerateSources.cmake") | ||
tools.replace_in_file(genSourcesMod, "flatcc::cli", "$ENV{FLATCC_CLI_EXE}", strict=True) |
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 do this in a patch before the build.
The script you're patching is only used in the samples and tests.
So when disabling those, the file is not used.
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.
Ok, I'll add a patch.
The file is also used by packages that use flatcc by the way.
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.
Maybe do a tools.replace (for the moment).
The branch will undergo more changes.
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 just added the patch...
if not self.options.runtime_lib_only: | ||
self.cpp_info.libs.append("flatcc%s" % debug_suffix) | ||
self.cpp_info.libs.append("flatccrt%s" % debug_suffix) | ||
self.cpp_info.components["cli"].names["cmake_find_package"] = "cli" |
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.
Maybe add a FIXME about missing flatcccli
and flatccruntime
(2 files) instead of one
Conan is missing this feature.
Also FIXME about missing flatcc::cli
target.
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.
Maybe add a FIXME about missing
flatcccli
andflatccruntime
(2 files) instead of one
Can you elaborate a bit? What exactly is missing ?
Also FIXME about missing
flatcc::cli
target
Ok, I'll add a comment that flatcc::cli
executable target currently doesn't work with Conan.
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.
Maybe it's unnecessary, but right now it's installing 3 cmake packages:
flatcc-config.cmake
, flatcccli-config.cmake
and flatccruntime-config.cmake. Of these, currently only
flatcc-config.cmake` is re-created by conan.
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.
But aren't flatcccli-config.cmake and flatccruntime-config.cmake more like 'internal' config files?
End-users should use find_package(flatcc).
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've documented flatcccli for cross building purposes.
But anyhow, maybe add a FIXME/TODO for when conan receives support for creating multiple cmake modules.
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've documented flatcccli for cross building purposes.
I noticed. Further down in the README there is also a section called 'Cross-compilation' that I am updating now. Will create a pull request to your branch when done tomorrow.
But anyhow, maybe add a FIXME/TODO for when conan receives support for creating multiple cmake modules.
Will do.
COMMAND cmake -E make_directory "${GEN_DIR}" | ||
COMMAND flatcc -a -o "${GEN_DIR}" "${FBS_DIR}/monster.fbs" | ||
DEPENDS flatcc "${FBS_DIR}/monster.fbs" | ||
if (MACOS_SIP_WORKAROUND) |
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.
Make sure that cmake in test_package/conanfile.py
is only run when not tools.cross_building(self.settings)
is true.
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.
Alternative is adding flatcc as build requirement in test_package/conanfile.py
, but then when increasing the version in conanfile.py you have to remember to update the flatcc version in test_package/conanfile.py as well.
@@ -6,6 +6,7 @@ from conans import ConanFile, CMake, tools, RunEnvironment
class FlatccTestConan(ConanFile):
settings = "os", "compiler", "build_type", "arch"
generators = "cmake"
+ build_requires = "flatcc/0.7.0pre"
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.
Wait, that won't work because the flatcc version in build_requires should be the same as the injected flatcc requirement.
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.
The test_package (=running flatcc::cli
) only works when not cross building.
I haven't tested, but maybe add
def build_requirements(self):
if tools.cross_building(self.settings):
self.build_requires("flatcc/{}"/format(some_version)
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.
But I don't think conan supports this.
I haven't seen recipes doing this. Maybe ask @jgsogo, the conan xbuild professional.
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 already tried to retrieve the flatcc version that is tested from self.requires (since Conan injects it for the test_package). But both in config_options() and configure() self.requires is empty.
I'll ask @jgsogo , else the following works when not using strange versions like 0.7.0pre:
build_requires = ("flatcc/[>=0.7.0]")
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.
That might work, but is not really reproducible.
There should be some way to know what package a test recipe is testing.
e.g. as self.test_package
that is available as soon as possible.
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.
Not a big fan of this interface, but it is documented: you can use self.deps_cpp_info["requirements"].version
in the build()
to get the version of the requirement (after the graph is resolved). Is this what you are looking for?
Conan injects the requirement
automatically, but after that line, it is a regular requirement.
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 @jgsogo ,
What we are looking for is finding out the version of the package that is being tested at a point where we can inject a build_requirement for the same package/version.
In order to build the test_package for flatcc (and similar tools) when cross-compiling, flatcc is also needed in the build architecture to generate the flatbuffer header files. Then the test executable is linked to the flatcc host arch library.
I suppose in the build() function it is too late to inject a build_requirement. Is it possible to do that earlier ?
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 see.
This is not possible today. The recipe in the test_package
works like any other recipe. The version of a requirement can be overridden by other branches of the graph (overrides, version-ranges, diamonds), so the final version of a requirement is only known when the graph is fully resolved and this is exactly the scenarios where the new validate()
function plays an important role.
BUT, I agree this is a test_package
and it totally makes sense to know the recipe being tested. And probably Conan should forbid modifying options in the test_package
too (conan-io/conan#7547). These (together with a proposal to test build-requires using the two-profiles approach conan-io/conan#7132) are things that we should explore before Conan v2.
Would you mind opening an issue with this feature request?
Meanwhile, I cannot see a documented way to achieve this behavior.
if (MACOS_SIP_WORKAROUND) | ||
set(INC_DIR "${PROJECT_SOURCE_DIR}/include") | ||
set(GEN_DIR "${CMAKE_CURRENT_BINARY_DIR}/generated") | ||
include_directories("${GEN_DIR}" "${INC_DIR}") |
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.
Is this SIP workaround still needed for the latest version?
flatcc does not depend on an external (shared) library.
And I made sure to set a relative RPATH on the executable.
Maybe change the if to include a version comparison/conditional?
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.
Flatcc depends on the flatcc compiler shared lib when configuring with BUILD_SHARED_LIBS=On. The SIP workaround is only used then:
if tools.os_info.is_macos and self.options["flatcc"].shared:
Its very well possible that the workaround is not needed anymore now that RPATH is set but at the moment I don't have access to a Mac that is recent enough to have SIP. I can check next week at work.
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.
Please do, or just let c3i test it for you 😛
SIP causes problems only when it needs external DYLD_LIBRARY_PATH
, which is not the case here.
Unless cmake is configured to not set RPATH on install, which is possible:
https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_INSTALL_RPATH.html
Co-authored-by: Anonymous Maarten <[email protected]>
Co-authored-by: Anonymous Maarten <[email protected]>
Co-authored-by: Anonymous Maarten <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This comment has been minimized.
This comment has been minimized.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This PR is waiting for merge of flatcc PR #171. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
It looks like dvidelabs/flatcc#171 is still being considered. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Still waiting on upstream.. |
Failure in build 12 (
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. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
This pull request has been automatically closed because it has not had recent activity. Thank you for your contributions. |
Specify library name and version: flatcc/0.6.0
Resolves #3907
conan-center hook activated.