-
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
[docs] Tweak package_templates #22325
Conversation
url: | ||
- "https://mirror1.net/package-1.1.0.tar.gz" | ||
- "https://mirror2.net/package-1.1.0.tar.gz" | ||
url: "https://mirror2.net/package-1.1.0.tar.gz" |
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.
To provide an example for newcomers of the more common non-list format for the URLs.
@@ -22,13 +22,13 @@ class PackageConan(ConanFile): | |||
name = "package" | |||
description = "short description" | |||
# Use short name only, conform to SPDX License List: https://spdx.org/licenses/ | |||
# In case not listed there, use "LicenseRef-<license-file-name>" | |||
# In case not listed there, use "DocumentRef-<license-file-name>:LicenseRef-<package-name>" |
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 DocumentRef
format has been recommended in all recent PRs where a non-standard license has been used.
@@ -62,7 +62,6 @@ def _settings_build(self): | |||
return getattr(self, "settings_build", self.settings) | |||
|
|||
# no exports_sources attribute, but export_sources(self) method instead | |||
# this allows finer grain exportation of patches per 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.
A bit confusing and outdated, imo.
# prefer self.requires method instead of requires attribute | ||
# Prefer self.requires method instead of requires attribute | ||
# Set transitive_headers=True (which usually also requires transitive_libs=True) if | ||
# the dependency is used in any of the packaged header 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.
I considered adding something like "Including a comment with a link to the the public header file using the dependency is encouraged." but it seemed a bit too wordy for me.
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.
As this is a template, I'd try to write that even if wordy, I think it's worth the space.
Maybe have 2 requires one without and one with traits, and leave the traits bit for the second one?
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.
Keeping all the explanation in the same line is more confusing, do it in separated ways. Keep the previous comment, then, add a new dummy requirement that requires transitive flags and add a dummy comment like: "used by foo/baz.hpp:34"
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.
Good idea. Thanks!
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.
Done.
def build_requirements(self): | ||
# only if we have to call autoreconf | ||
self.tool_requires("libtool/x.y.z") | ||
self.tool_requires("libtool/2.4.7") |
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.
While the versions should be always checked and updated, the libtool
and automake
versions change very rarely, so it does not hurt to include them for convenience.
tc = AutotoolsDeps(self) | ||
tc.generate() | ||
# some recipes might require a workaround for MSVC: | ||
# https://github.com/conan-io/conan-center-index/blob/00ce907b910d0d772f1c73bb699971c141c423c1/recipes/xapian-core/all/conanfile.py#L106-L135 |
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 is an unfortunately rather frequently required workaround. I think it's beneficial to include it as (a) the ./configure error is just a cryptic "cannot create C executables" and (b) the fix is not at all obvious.
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, add a related issue too to be more visible about that idiom and will be done in the future.
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.
Done.
@@ -158,7 +158,7 @@ def generate(self): | |||
env.define("CC", f"{compile_wrapper} cl -nologo") | |||
env.define("CXX", f"{compile_wrapper} cl -nologo") | |||
env.define("LD", "link -nologo") | |||
env.define("AR", f"{ar_wrapper} \"lib -nologo\"") |
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 -nologo
option saves a few spammy lines from the build logs, but it has caused the build to fail with a "could not determine ar interface" in several recipes for me, so it's safer to not include it.
|
||
def source(self): | ||
get(self, **self.conan_data["sources"][self.version], strip_root=True) | ||
|
||
def generate(self): | ||
# BUILD_SHARED_LIBS and POSITION_INDEPENDENT_CODE are automatically parsed when self.options.shared or self.options.fPIC exist | ||
# BUILD_SHARED_LIBS and POSITION_INDEPENDENT_CODE are set automatically as tc.variables when self.options.shared or self.options.fPIC exist | ||
# Note that tc.variables require either cmake_minimum_required() >= 3.13 or the CMP0077 policy set to NEW to work correctly. |
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 is what prompted me to open this PR in the first place. It's a huge pitfall for newcomers, which can cause silent and non-obvious failures if it is not accounted for.
tc = CMakeToolchain(self) | ||
# Boolean values are preferred instead of "ON"/"OFF" | ||
tc.variables["PACKAGE_CUSTOM_DEFINITION"] = True | ||
tc.variables["PACKAGE_BUILD_TESTS"] = False |
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.
A slightly more relevant example.
def build_requirements(self): | ||
self.tool_requires("tool/x.y.z") | ||
self.tool_requires("cmake/[>=3.16 <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.
A slightly more typical example for the CMake recipe.
if self.dependencies["dependency"].options.foobar: | ||
tc.variables["DEPENDENCY_LIBPATH"] = self.dependencies["dependency"].cpp_info.libdirs | ||
# cache_variables should be used sparingly, example setting cmake policies | ||
tc.variables["DEPENDENCY_LIBPATH"] = self.dependencies["dependency"].cpp_info.libdir.replace("\\", "/") |
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.
Need to convert to forward slashes on Windows for CMake not to interpret the backslashes as escaped characters.
# don't use self.settings.compiler.runtime | ||
tc.variables["USE_MSVC_RUNTIME_LIBRARY_DLL"] = not is_msvc_static_runtime(self) | ||
# deps_cpp_info, deps_env_info and deps_user_info are no longer 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.
No longer relevant with Conan 2.0 being commonplace.
# disable subdirectories by truncating their CMakeLists.txt | ||
save(self, os.path.join(self.source_folder, "tests", "CMakeLists.txt"), "") |
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.
test
and examples
directories need to be disabled quite often and this approach requires less maintenance overhead compared to patching CMakeLists.txt
.
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 would not include it. It encourages people doing more patches instead of trying to check it with the upstream an alternative. Remember, patches are the last resource always.
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.
My worry with this as explained elsewhere is that in the case where a new version changes how their tests etc are set up, we won't fail as it would have happened with patches, thus making it easier to sneak tests and the like without noticing. wdyt?
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.
Why not both - use this as a patch and submit a fix upstream as well? 😁
Using plain patches is better in principle, but the add_subdirectory()
statements are typically one of the very last ones in a CMakeLists.txt
file, which for larger projects means that this needs to be updated for almost every single release due to the line numbers shifting. I can remove this example here, but in general, would a replace_in_file()
with strict=True
be acceptable instead?
@@ -139,9 +144,7 @@ def package(self): | |||
rmdir(self, os.path.join(self.package_folder, "lib", "pkgconfig")) | |||
rmdir(self, os.path.join(self.package_folder, "lib", "cmake")) | |||
rmdir(self, os.path.join(self.package_folder, "share")) | |||
rm(self, "*.la", os.path.join(self.package_folder, "lib")) |
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 have never encountered a *.la
file in a CMake project.
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.
good catch
# To export additional CMake variables, such as upper-case variables otherwise set by the project's *-config.cmake, | ||
# you can copy or save a .cmake file under <prefix>/lib/cmake/ with content like | ||
# set(XYZ_VERSION ${${CMAKE_FIND_PACKAGE_NAME}_VERSION}) | ||
# set(XYZ_INCLUDE_DIRS ${${CMAKE_FIND_PACKAGE_NAME}_INCLUDE_DIRS}) | ||
# ... | ||
# and set the following fields: | ||
self.cpp_info.builddirs.append(os.path.join("lib", "cmake")) | ||
cmake_module = os.path.join("lib", "cmake", "conan-official-variables.cmake") | ||
self.cpp_info.set_property("cmake_build_modules", [cmake_module]) | ||
self.cpp_info.build_modules["cmake_find_package"] = [cmake_module] | ||
self.cpp_info.build_modules["cmake_find_package_multi"] = [cmake_module] |
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.
Matching the *-config.cmake
output created by the project is often overlooked, imo, and it's difficult to find a good example on how to handle this as a newbie.
${CMAKE_FIND_PACKAGE_NAME}
is preferable over a hard-coded package name as it allows the name to be overriden in CMakeDeps without breaking things.
basic_layout(self, src_folder="src") | ||
|
||
def requirements(self): | ||
# Prefer self.requires method instead of requires attribute | ||
# Direct dependencies of header only libs are always transitive since they are included in public headers | ||
self.requires("dependency/0.8.1", transitive_headers=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.
transitive_headers=True
is already the default for package_type = "header-library"
.
platform_toolset = MSBuildToolchain(self).toolset | ||
import_conan_generators = "" | ||
for props_file in ["conantoolchain.props", "conandeps.props"]: | ||
props_path = os.path.join(self.generators_folder, props_file) | ||
if os.path.exists(props_path): | ||
import_conan_generators += f"<Import Project=\"{props_path}\" />" | ||
for vcxproj_file in vcxproj_files: | ||
for vcxproj_file in self.source_path.rglob("*.vcxproj"): |
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.
Does not work 100% of the time, but usually does, and is more convenient than explicitly listing all of the vcxproj 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.
Any example where it does not 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.
Don't remember any, really.
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.
looking good, some observations.
@@ -78,11 +77,12 @@ def configure(self): | |||
self.settings.rm_safe("compiler.libcxx") | |||
|
|||
def layout(self): | |||
# src_folder must use the same source folder name the project |
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, keep this comment, it explains why do we use src_folder="src"
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.
A comment would not hurt, but the current wording is kind of broken, imo. Could you provide a better one, maybe?
# prefer self.requires method instead of requires attribute | ||
# Prefer self.requires method instead of requires attribute | ||
# Set transitive_headers=True (which usually also requires transitive_libs=True) if | ||
# the dependency is used in any of the packaged header 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.
Keeping all the explanation in the same line is more confusing, do it in separated ways. Keep the previous comment, then, add a new dummy requirement that requires transitive flags and add a dummy comment like: "used by foo/baz.hpp:34"
# only if upstream configure.ac relies on PKG_CHECK_MODULES macro | ||
if not self.conf.get("tools.gnu:pkg_config", check_type=str): | ||
self.tool_requires("pkgconf/x.y.z") | ||
self.tool_requires("pkgconf/2.1.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.
please, revert to x.y.z. Otherwise, people will not check the latest version available when using the template.
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. The Meson recipe template already uses real versions, though. Should I blank these out as well?
@@ -114,20 +114,18 @@ def build_requirements(self): | |||
# for msvc support to get compile & ar-lib scripts (may be avoided if shipped in source code of the library) | |||
# not needed if libtool already in build requirements | |||
if is_msvc(self): | |||
self.tool_requires("automake/x.y.z") |
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.
ditto.
tc = AutotoolsDeps(self) | ||
tc.generate() | ||
# some recipes might require a workaround for MSVC: | ||
# https://github.com/conan-io/conan-center-index/blob/00ce907b910d0d772f1c73bb699971c141c423c1/recipes/xapian-core/all/conanfile.py#L106-L135 |
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, add a related issue too to be more visible about that idiom and will be done in the future.
|
||
# In case there are dependencies listed under requirements, CMakeDeps should be used | ||
deps = CMakeDeps(self) | ||
# You can override the CMake package and target names if they don't match the names used in the project |
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.
nice addition.
# disable subdirectories by truncating their CMakeLists.txt | ||
save(self, os.path.join(self.source_folder, "tests", "CMakeLists.txt"), "") |
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 would not include it. It encourages people doing more patches instead of trying to check it with the upstream an alternative. Remember, patches are the last resource always.
@@ -139,9 +144,7 @@ def package(self): | |||
rmdir(self, os.path.join(self.package_folder, "lib", "pkgconfig")) | |||
rmdir(self, os.path.join(self.package_folder, "lib", "cmake")) | |||
rmdir(self, os.path.join(self.package_folder, "share")) | |||
rm(self, "*.la", os.path.join(self.package_folder, "lib")) |
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.
good catch
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 a lot for this @valgur, we were due taking a look at this. I've some comments on some of the changes, otherwise it's great, thanks!!
# prefer self.requires method instead of requires attribute | ||
# Prefer self.requires method instead of requires attribute | ||
# Set transitive_headers=True (which usually also requires transitive_libs=True) if | ||
# the dependency is used in any of the packaged header 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.
As this is a template, I'd try to write that even if wordy, I think it's worth the space.
Maybe have 2 requires one without and one with traits, and leave the traits bit for the second one?
# disable subdirectories by truncating their CMakeLists.txt | ||
save(self, os.path.join(self.source_folder, "tests", "CMakeLists.txt"), "") |
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.
My worry with this as explained elsewhere is that in the case where a new version changes how their tests etc are set up, we won't fail as it would have happened with patches, thus making it easier to sneak tests and the like without noticing. wdyt?
# prefer self.requires method instead of requires attribute | ||
# Prefer self.requires method instead of requires attribute | ||
# Set transitive_headers=True (which usually also requires transitive_libs=True) if | ||
# the dependency is used in any of the packaged header files. | ||
self.requires("dependency/0.8.1") |
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 an example of a version range fake dependency, which a comment linking to the FAQ for the allowed ones?
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.
Done.
Also, btw, should the zlib
dependency be added to the allowed list in https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/dependencies.md#version-ranges, maybe?
platform_toolset = MSBuildToolchain(self).toolset | ||
import_conan_generators = "" | ||
for props_file in ["conantoolchain.props", "conandeps.props"]: | ||
props_path = os.path.join(self.generators_folder, props_file) | ||
if os.path.exists(props_path): | ||
import_conan_generators += f"<Import Project=\"{props_path}\" />" | ||
for vcxproj_file in vcxproj_files: | ||
for vcxproj_file in self.source_path.rglob("*.vcxproj"): |
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.
Any example where it does not work?
This comment has been minimized.
This comment has been minimized.
07e1cd6
to
7ce0494
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Conan v1 pipeline ❌Changes not allowed in build 1:
Changing recipes and configuration/docs files in the same pull-request is not allowed. Please, split changes into several pull-requests. 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:Changes not allowed in build 1:
Changing recipes and configuration/docs files in the same pull-request is not allowed. Please, split changes into several pull-requests. |
It keeps the logs slightly cleaner for a very infrequent command but has caused issues with "ar interface detection" in ./configure for several recipes.
These change very infrequently (except for pkgconf, somewhat). I think these can be included for convenience.
…ckage The test_package directory gets gunked up with generated VirtualBuildEnv files otherwise.
b56cccc
to
f9cfa69
Compare
…ator props The CMake and PkgConfig info is the most relevant part to any consumers and should come first, imo. New recipes should not bother with the legacy generators, especially after test_v1_package is no longer included.
Adds notes for a few important but missing details, which I have encountered when creating or updating packages.
Removes a few outdated comments and improves the wording a bit of several others.
I'll add comments for specific changes as review comments.