conan-io / docs

conan.io reStructuredText documentation
http://docs.conan.io
MIT License
104 stars 352 forks source link

`self.settings` vs `self.info.settings` in `validate`/`validate_build` #2792

Open SSE4 opened 1 year ago

SSE4 commented 1 year ago

right now, documentation is inconsistent, e.g.: https://docs.conan.io/en/latest/reference/conanfile/methods.html#validate https://docs.conan.io/en/latest/reference/conanfile/methods.html#validate-build use just self.settings, while: https://docs.conan.io/en/latest/creating_packages/define_abi_compatibility.html uses self.info.settings. for newcomers, it's not clear, which one should be used and in that circumstances. I think documentation should be aligned with the same recommendation on all pages. if for some reason, both self.info.settings and plain self.settings could be used, it should have some disclaimer, describing the difference between them. see also https://github.com/conan-io/conan-center-index/pull/13754

uilianries commented 1 year ago

So far, what we have:

So there is a trick, which is not well documented yet, but at least, we tried to sane on ConanCenterIndex by its templates.

Still, the documentation need to be improved.

prince-chrismc commented 1 year ago

https://github.com/conan-io/conan/issues/12321 https://github.com/conan-io/conan/issues/12135

SSE4 commented 1 year ago

@uilianries maybe we can fill a table like that?

|     | validate      | validate           | validate_build | validate_build     |
|-----|---------------|--------------------|----------------|--------------------|
|     | self.settings | self.info.settings | self.settings  | self.info.settings |
| 1.x |               |                    |                |                    |
| 2.x |               |                    |                |                    |
uilianries commented 1 year ago

@SSE4 basically:

|     | validate      | validate           | validate_build | validate_build     |
|-----|---------------|--------------------|----------------|--------------------|
|     | self.settings | self.info.settings | self.settings  | self.info.settings |
| 1.x | header-only   | regular libraries  | ALWAYS         | NEVER              |
| 2.x | NEVER         | ALWAYS             | ALWAYS         | NEVER              |
SSE4 commented 1 year ago

I doubt it's that simple... regular package may erase settings as well (many erase libcxx, at least, several erase other settings like build_type or arch). there are libraries with header_only option (boost, fmt, etc.), and settings are cleared only for some package IDs. that makes it non-trivial to write a correct validate method.

IMO documentation should show how to write validate so it works for both v1 and v2 (same code, no conan_version condition). and it shouldn't make a difference for header-only vs "regular-libraries", as there are many corner cases (regular libraries erasing settings, regular libraries with header-only option, etc.).

prince-chrismc commented 1 year ago

That's the goal.

I think once the 1.54 and beta 5 drop we will have an answer to this that's consistent.

SSE4 commented 1 year ago

there is a proposal to be considered: