docs: split some policies by the implicated method #14211

prince-chrismc · 2022-11-15T19:12:21Z

Docs!

This is yet another step to #13673 - for this one I just want to create new files and removed only "packaging_policy".
The goal is to breakout methods into pairs to it's easy know where to look for related information.

Feedback welcomed - my goal is to close in on this branch https://github.com/prince-chrismc/conan-center-index/tree/docs/seperate-by-method which is my current plan

SSE4 · 2022-11-16T04:36:49Z

docs/adding_packages/build_and_package.md

+# Build and Package
+
+This document gathers all the relevant information regarding the general lines to follow while writing either the `build()` or the `package()` methods.
+Both of these methods often take advantage of build helpers build binaries and install to the `package_folder`.


why not have build.md + package.md?

There's not enough docs written to justify it yet 😄

SSE4 · 2022-11-16T04:37:47Z

docs/adding_packages/build_and_package.md

+
+* `build()` method should focus on build only, not installation. During the build, nothing should be written in `package` folder. Installation step should only occur in `package()` method.
+
+* The build itself should only rely on local files. Nothing should be downloaded from internet during this step. If external files are required, they should come from `requirements` or `build_requirements`, in addition to source files downloaded in `source()` or coming from recipe itself.


if think if sources are different per platform, we do downloads in build method (e.g. android-ndk).

Thats not the case - theres plenty of those and they work in source() --- maybe I misunderstood you?

Why would it not work?

that's the case https://github.com/conan-io/conan-center-index/blob/master/recipes/android-ndk/all/conanfile.py#L57-L67

Thats downloading pre compiled tooling which is why it's in the build method -- not the arch specific nature in this case

fair, but we have more cases like that (not just for pre-built tools):

conan-center-index/recipes/cspice/all/conanfile.py

Lines 92 to 111 in e907fe2

def _get_sources(self):

with chdir(self, self._parent_source_folder):

host_os = self._get_os_or_subsystem()

compiler = str(self.settings.compiler)

arch = str(self.settings.arch)

data = self.conan_data["sources"][self.version][host_os][compiler][arch]

url = data["url"]

if url.endswith(".tar.Z"): # Python doesn't have any module to uncompress .Z files

tarball = os.path.basename(url)

download(self, url, tarball, sha256=data["sha256"])

self.run(f"zcat {tarball} | tar -xf -")

os.remove(tarball)

else:

get(self, **data)

rmdir(self, self.source_folder)

rename(self, "cspice", os.path.basename(self.source_folder))

def build(self):

self._get_sources()

apply_conandata_patches(self)

SSE4 · 2022-11-16T04:38:03Z

docs/adding_packages/build_and_package.md

+
+* `build()` method should focus on build only, not installation. During the build, nothing should be written in `package` folder. Installation step should only occur in `package()` method.
+
+* The build itself should only rely on local files. Nothing should be downloaded from internet during this step. If external files are required, they should come from `requirements` or `build_requirements`, in addition to source files downloaded in `source()` or coming from recipe itself.


Suggested change

* The build itself should only rely on local files. Nothing should be downloaded from internet during this step. If external files are required, they should come from `requirements` or `build_requirements`, in addition to source files downloaded in `source()` or coming from recipe itself.

* The build itself should only rely on local files. Nothing should be downloaded from internet during this step. If external files are required, they should come from `requirements` or `tool_requirements`, in addition to source files downloaded in `source()` or coming from recipe itself.

That's consufising the method is called build_requirements which is what sentence is referring too I think

I suppose we need to avoid confusion and use consistent naming/terminology across the docs. either tool_requires everywhere, or build_requires everywhere.

SSE4 · 2022-11-16T04:39:29Z

docs/adding_packages/build_and_package.md

+
+* It is forbidden to run other conan client commands during build. In other words, if upstream build files call conan under the hood (through `cmake-conan` for example or any other logic), this logic must be removed.
+
+* Settings from profile should be honored (`build_type`, `compiler.libcxx`, `compiler.cppstd`, `compiler.runtime` etc).


Suggested change

* Settings from profile should be honored (`build_type`, `compiler.libcxx`, `compiler.cppstd`, `compiler.runtime` etc).

* Settings from profile should be honored (`build_type`, 'os', 'arch', `compiler.libcxx`, `compiler.cppstd`, `compiler.runtime` etc).

This was intended to call out problematic settings not all of them 🤔

I am not sure which are more problematic 🤔

SSE4 · 2022-11-16T04:40:30Z

docs/adding_packages/build_and_package.md

+
+* Settings from profile should be honored (`build_type`, `compiler.libcxx`, `compiler.cppstd`, `compiler.runtime` etc).
+
+* These env vars from profile should be honored and properly propagated to underlying build system during the build: `CC`, `CXX`, `CFLAGS`, `CXXFLAGS`, `LDFLAGS`.


it deserves an example. for newcomers, it's kinda unclear. can you illustrate how to honor these variables, e.g. for CMake?

Yes the docs are bad - I am not trying to fix them in this PR 😄 I got many more too come!

SSE4 · 2022-11-16T04:41:11Z

docs/adding_packages/build_and_package.md

+
+* It is forbidden to run other conan client commands during build. In other words, if upstream build files call conan under the hood (through `cmake-conan` for example or any other logic), this logic must be removed.
+
+* Settings from profile should be honored (`build_type`, `compiler.libcxx`, `compiler.cppstd`, `compiler.runtime` etc).


same, it deserves an example. for newcomers, it's kinda unclear, how to to it exactly. it would be nice if you illustrate with a small code block, e.g. for CMake.

docs/adding_packages/build_and_package.md

SSE4 · 2022-11-16T04:43:42Z

docs/adding_packages/build_and_package.md

+
+* pkg-config files must be removed (they will be generated for consumers by `pkg_config` or `PkgConfigDeps` generators).
+
+* On *nix systems, executables and shared libraries should have empty `RPATH`/`RUNPATH`/`LC_RPATH`.


how do I achieve that? how do I ensure that? examples are needed.

SSE4 · 2022-11-16T04:43:49Z

docs/adding_packages/build_and_package.md

+
+* On *nix systems, executables and shared libraries should have empty `RPATH`/`RUNPATH`/`LC_RPATH`.
+
+* On macOS, install name in `LC_ID_DYLIB` section of shared libs must be `@rpath/<libfilename>`.


how do I achieve that? how do I ensure that? examples are needed.

SSE4 · 2022-11-16T04:44:35Z

docs/adding_packages/build_and_package.md

+
+* On macOS, install name in `LC_ID_DYLIB` section of shared libs must be `@rpath/<libfilename>`.
+
+* Installed files must not contain absolute paths specific to build machine. Relative paths to other packages is also forbidden since relative paths of dependencies during build may not be the same for consumers. Hardcoded relative paths pointing to a location in the package itself are allowed.


how do I check for that? how do I prevent/fix that? examples are needed.

docs/adding_packages/dependencies.md

docs/adding_packages/sources_and_patches.md

SSE4 · 2022-11-16T08:30:18Z

docs/adding_packages/build_and_package.md

+
+* pkg-config files must be removed (they will be generated for consumers by `pkg_config` or `PkgConfigDeps` generators).
+
+* On *nix systems, executables and shared libraries should have empty `RPATH`/`RUNPATH`/`LC_RPATH`.


we also need to put some links to the relevant documentation for:

RPATH

RUNPATH

LC_RPATH

LC_ID_DYLIB

@rpath

maybe with some brief examples on how to inspect them (commands like objdump, readelf, otool, etc.).
these are very technical and low-level things, we cannot expect all the readers to know about them.

SSE4 · 2022-11-16T08:31:50Z

docs/adding_packages/build_and_package.md

+
+* Except CMake and a working build toolchain (compiler, linker, archiver etc), the recipe should not assume that any other build tool is installed on user build machine (like Meson, autotools or pkg-config). On Windows, recipe should not assume that a shell is available (like MSYS2). Therefore, if the build requires additional tools, they should be added to `build_requirements()`.
+
+* It is forbidden to run other conan client commands during build. In other words, if upstream build files call conan under the hood (through `cmake-conan` for example or any other logic), this logic must be removed.


how do I check for that?

docs/adding_packages/dependencies.md

SSE4 · 2022-11-16T08:33:33Z

docs/adding_packages/dependencies.md

+  symbols, it may be acceptable.
+* Only other ConanCenter recipes are allowed in `requires`/`requirements()` and `build_requires`/`build_requirements()`.
+* If a requirement is conditional, this condition must not depend on build context. Build requirements don't have this constraint.
+* Forcing options of dependencies inside a recipe should be avoided, except if it is mandatory for the library - in which case it must


example would be nice

SSE4 · 2022-11-16T08:34:11Z

docs/adding_packages/sources_and_patches.md

@@ -0,0 +1,33 @@
+# Sources and Patches
+
+This documents contains everything related to the `source()`. This includes picking sources, where they should come from and goes into when and how to modify sources.


Suggested change

This documents contains everything related to the `source()`. This includes picking sources, where they should come from and goes into when and how to modify sources.

This documents contains everything related to the `source()`. This includes picking sources, where they should come from.

in another section, we already stated sources are immutable

docs/adding_packages/sources_and_patches.md

SSE4 · 2022-11-16T08:36:01Z

docs/adding_packages/sources_and_patches.md

+* Library sources should come from an official origin like the library source code repository or the official
+release/download webpage.
+
+* If an official source archive is available, it should be preferred over an auto-generated archive.


maybe we should explain it a little bit more, on how to distinguish these two.
e.g., in GitHub projects, prefer archives from the releases tab, rather code - download zip

SSE4 · 2022-11-16T08:36:29Z

docs/adding_packages/sources_and_patches.md

+
+**Source immutability:** Downloaded source code stored under `source` folder should not be modified. Any patch should be applied to the copy of this source code when a build is executed (basically in `build()` method).
+
+**Building from sources:** Recipes should always build packages from library sources.


we have some known exceptions to this rule (MSYS2, Android NDK, etc.)

SSE4 · 2022-11-16T08:37:00Z

docs/adding_packages/sources_and_patches.md

+
+* Library sources that are not publicly available will not be allowed in this repository even if the license allows their redistribution.
+
+* If library sources cannot be downloaded from their official origin or cannot be consumed directly due to their


we had some special cases for that, e.g. for gSOAP. maybe explain it here.

SpaceIm · 2022-11-16T10:42:39Z

I believe that:

one section should list all policies and give them an id.
another section could list hooks associated to each policy
and another one with technical advices to honor these policies in recipes

prince-chrismc · 2022-11-16T17:42:31Z

@SSE4 I really appreciate the detailed and quality review but I have no intention of fixing the docs in this PR - my goal is to simply just move them. Once the structure is improved and things are grouped we can worry about fixing them.

Right now it's very difficult to reason about them because the examples you are suggesting already exist just in a place that does not make sense -- please see #13673 I just want to move them right now so I will not be applying most of ur suggestions as of now.

You suggestion are really good and I will note them for my future self 👍

prince-chrismc · 2022-11-16T17:51:26Z

@SpaceIm I love the way you are thinking!

I've gathered a lot of feedback and most contributors do not like the current system (which is have a super long list of rules which are numbered -- aka the hooks). In the long run we need to have self documenting linter rules that annotate the code and suggest solutions. No more have the same information on three different pages saying different things.

I have an open ticket to start a list of linter "best practices" we should add - think conventions we already have (like how we write the source method) - for now you can put your ideas in #11393 and we'll consider them 😄

My goal is delete some of the docs (in like 6-8 months time frame) and replace them with linters - a prime example is the "you should order methods in the way they are called" which is broken almost everywhere.

I absolutely agree we need more linters - and they should explain themselves with nice annotations.

prince-chrismc · 2022-11-16T19:44:38Z

Sorry about the force push - the merge conflicts did not like me

SSE4 · 2022-11-17T05:11:42Z

Right now it's very difficult to reason about them because the examples you are suggesting already exist just in a place that does not make sense -- please see #13673 I just want to move them right now so I will not be applying most of ur suggestions as of now.

if examples are already there, why not put links to them? otherwise, it's hard to discover corresponding examples for newcomers.

SSE4

docs need more examples

franramirez688

Overall, I think it's a good job! 👏 For newbies, it could not be enough to understand everything, but it's a good start to keep improving this current documentation. Looking forward to the following PRs!

EDITED: my comments are only suggestions about punctuation, clarity, etc.

docs/adding_packages/build_and_package.md

docs/adding_packages/dependencies.md

Co-authored-by: Francisco Ramírez <[email protected]> Co-authored-by: SSE4 <[email protected]>

…ethods

prince-chrismc · 2022-11-21T16:18:59Z

docs need more examples

I am not fixing the docs in this PR. I have many other PRs waiting -- I just would like to go through them in smaller bit size pieces so everyone has time to adjust.

Every single change here means I spent 30-45 min fixing my 3 other branches and it's not fun having merge conflicts.

uilianries

LGTM

prince-chrismc mentioned this pull request Nov 15, 2022

Docs: Improved Structure for "adding packages" to help contributors find information #13673

Closed

10 tasks

github-actions bot mentioned this pull request Nov 15, 2022

Pull Requests Ready to be Reviewed prince-chrismc/conan-center-index-pending-review#1

Open

conan-center-bot added the Docs label Nov 15, 2022

SSE4 reviewed Nov 16, 2022

View reviewed changes

docs/adding_packages/build_and_package.md Outdated Show resolved Hide resolved

SSE4 reviewed Nov 16, 2022

View reviewed changes

docs/adding_packages/build_and_package.md Outdated Show resolved Hide resolved

SSE4 reviewed Nov 16, 2022

View reviewed changes

docs/adding_packages/dependencies.md Outdated Show resolved Hide resolved

SSE4 reviewed Nov 16, 2022

View reviewed changes

docs/adding_packages/dependencies.md Outdated Show resolved Hide resolved

SSE4 reviewed Nov 16, 2022

View reviewed changes

docs/adding_packages/dependencies.md Outdated Show resolved Hide resolved

SSE4 reviewed Nov 16, 2022

View reviewed changes

docs/adding_packages/sources_and_patches.md Outdated Show resolved Hide resolved

SSE4 reviewed Nov 16, 2022

View reviewed changes

docs/adding_packages/dependencies.md Outdated Show resolved Hide resolved

SSE4 reviewed Nov 16, 2022

View reviewed changes

docs/adding_packages/sources_and_patches.md Outdated Show resolved Hide resolved

SSE4 reviewed Nov 16, 2022

View reviewed changes

docs: split some policies by the implicated method

b4db96a

prince-chrismc force-pushed the docs/renames-by-methods branch from df6451a to b4db96a Compare November 16, 2022 19:43

Chris Mc added 2 commits November 16, 2022 11:47

fix broken link

c457b55

fix broken link

e7e9452

conan-center-bot added the GitHub config label Nov 16, 2022

conan-center-bot requested a review from SSE4 November 17, 2022 12:00

SSE4 suggested changes Nov 17, 2022

View reviewed changes

franramirez688 previously approved these changes Nov 17, 2022

View reviewed changes

Apply suggestions from code review

c1a1d12

Co-authored-by: Francisco Ramírez <[email protected]> Co-authored-by: SSE4 <[email protected]>

prince-chrismc dismissed franramirez688’s stale review via c1a1d12 November 17, 2022 16:25

Merge remote-tracking branch 'upstream/master' into docs/renames-by-m…

a5a3dd3

…ethods

uilianries approved these changes Nov 21, 2022

View reviewed changes

franramirez688 approved these changes Nov 21, 2022

View reviewed changes

conan-center-bot merged commit e70b881 into conan-io:master Nov 21, 2022


		* `build()` method should focus on build only, not installation. During the build, nothing should be written in `package` folder. Installation step should only occur in `package()` method.

		* The build itself should only rely on local files. Nothing should be downloaded from internet during this step. If external files are required, they should come from `requirements` or `build_requirements`, in addition to source files downloaded in `source()` or coming from recipe itself.

	def _get_sources(self):
	with chdir(self, self._parent_source_folder):
	host_os = self._get_os_or_subsystem()
	compiler = str(self.settings.compiler)
	arch = str(self.settings.arch)
	data = self.conan_data["sources"][self.version][host_os][compiler][arch]
	url = data["url"]
	if url.endswith(".tar.Z"): # Python doesn't have any module to uncompress .Z files
	tarball = os.path.basename(url)
	download(self, url, tarball, sha256=data["sha256"])
	self.run(f"zcat {tarball} \| tar -xf -")
	os.remove(tarball)
	else:
	get(self, **data)
	rmdir(self, self.source_folder)
	rename(self, "cspice", os.path.basename(self.source_folder))

	def build(self):
	self._get_sources()
	apply_conandata_patches(self)


		* It is forbidden to run other conan client commands during build. In other words, if upstream build files call conan under the hood (through `cmake-conan` for example or any other logic), this logic must be removed.

		* Settings from profile should be honored (`build_type`, `compiler.libcxx`, `compiler.cppstd`, `compiler.runtime` etc).


		* Settings from profile should be honored (`build_type`, `compiler.libcxx`, `compiler.cppstd`, `compiler.runtime` etc).

		* These env vars from profile should be honored and properly propagated to underlying build system during the build: `CC`, `CXX`, `CFLAGS`, `CXXFLAGS`, `LDFLAGS`.


		* pkg-config files must be removed (they will be generated for consumers by `pkg_config` or `PkgConfigDeps` generators).

		* On *nix systems, executables and shared libraries should have empty `RPATH`/`RUNPATH`/`LC_RPATH`.


		* On *nix systems, executables and shared libraries should have empty `RPATH`/`RUNPATH`/`LC_RPATH`.

		* On macOS, install name in `LC_ID_DYLIB` section of shared libs must be `@rpath/<libfilename>`.


		* On macOS, install name in `LC_ID_DYLIB` section of shared libs must be `@rpath/<libfilename>`.

		* Installed files must not contain absolute paths specific to build machine. Relative paths to other packages is also forbidden since relative paths of dependencies during build may not be the same for consumers. Hardcoded relative paths pointing to a location in the package itself are allowed.


		* Except CMake and a working build toolchain (compiler, linker, archiver etc), the recipe should not assume that any other build tool is installed on user build machine (like Meson, autotools or pkg-config). On Windows, recipe should not assume that a shell is available (like MSYS2). Therefore, if the build requires additional tools, they should be added to `build_requirements()`.

		* It is forbidden to run other conan client commands during build. In other words, if upstream build files call conan under the hood (through `cmake-conan` for example or any other logic), this logic must be removed.

		@@ -0,0 +1,33 @@
		# Sources and Patches

		This documents contains everything related to the `source()`. This includes picking sources, where they should come from and goes into when and how to modify sources.


		Source immutability: Downloaded source code stored under `source` folder should not be modified. Any patch should be applied to the copy of this source code when a build is executed (basically in `build()` method).

		Building from sources: Recipes should always build packages from library sources.


		* Library sources that are not publicly available will not be allowed in this repository even if the license allows their redistribution.

		* If library sources cannot be downloaded from their official origin or cannot be consumed directly due to their

docs: split some policies by the implicated method #14211

docs: split some policies by the implicated method #14211

Conversation

prince-chrismc commented Nov 15, 2022

SSE4 Nov 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SpaceIm commented Nov 16, 2022

prince-chrismc commented Nov 16, 2022

prince-chrismc commented Nov 16, 2022

prince-chrismc commented Nov 16, 2022

SSE4 commented Nov 17, 2022

SSE4 left a comment

Choose a reason for hiding this comment

franramirez688 left a comment • edited Loading

Choose a reason for hiding this comment

prince-chrismc commented Nov 21, 2022

uilianries left a comment

Choose a reason for hiding this comment

SSE4 Nov 16, 2022 •

edited

Loading

franramirez688 left a comment •

edited

Loading