Skip to content
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: capture our policy for the options shared and fPIC #12281

Merged
merged 14 commits into from
Sep 21, 2022

Conversation

prince-chrismc
Copy link
Contributor

@prince-chrismc prince-chrismc commented Aug 16, 2022

Docs!

This came as a question on Slack, thought it was worth writing down

https://cpplang.slack.com/archives/C41CWV9HA/p1660683315599809

@conan-center-bot

This comment has been minimized.

@jgsogo
Copy link
Contributor

jgsogo commented Aug 17, 2022

IMHO, there is some overlapping between this section and https://github.com/conan-io/conan-center-index/blob/master/docs/packaging_policy.md#options, we can argue that one is targeting reviewers while the other is more for recipe authors... but in the end both are the same people and both sections should contain the same message. Maybe it is time to merge those sections and update them, wdyt?

@uilianries
Copy link
Member

Yes. Indeed is a sad situation when you are a newbie knows nothing about it and need a review to discover default values and other stuff. Merging sections sounds good.

@prince-chrismc
Copy link
Contributor Author

Maybe it is time to merge those sections and update them, wdyt?

Given I was not able to find these docs... I think it's worth giving it some TLC and combining things together with "how to add package" might be the next step

@prince-chrismc prince-chrismc marked this pull request as draft August 17, 2022 18:48
@conan-center-bot

This comment has been minimized.

@prince-chrismc prince-chrismc marked this pull request as ready for review September 8, 2022 16:21
@prince-chrismc
Copy link
Contributor Author

I've decided to revive this since conference are making my other idea take longer then expected

uilianries
uilianries previously approved these changes Sep 9, 2022
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, the note about the default value of shared option for other packages is really important.

docs/packaging_policy.md Outdated Show resolved Hide resolved
docs/packaging_policy.md Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

docs/packaging_policy.md Outdated Show resolved Hide resolved
@SSE4 SSE4 requested review from jgsogo and uilianries September 14, 2022 15:26
docs/packaging_policy.md Outdated Show resolved Hide resolved
if self.options.shared:
try:
del self.options.fPIC
except Exception:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, Conan 1.53 will bring the new method rm_safe (also for settings) so it'll be possible to avoid those try-except structures. It'll look like this:

   def configure(self):
      if self.options.shared:
         self.options.rm_safe("fPIC")

docs/packaging_policy.md Outdated Show resolved Hide resolved
docs/packaging_policy.md Outdated Show resolved Hide resolved
docs/packaging_policy.md Outdated Show resolved Hide resolved
Co-authored-by: Francisco Ramírez <[email protected]>
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor Author

@prince-chrismc prince-chrismc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for holding my hand 💟 I think I finally captured the important parts

docs/packaging_policy.md Outdated Show resolved Hide resolved
docs/packaging_policy.md Outdated Show resolved Hide resolved
@conan-center-bot

This comment has been minimized.


Recipes with such option should include the following in their `package_id` method

```python
def package_id(self):
if self.options.header_only:
self.info.header_only()
self.info.clear()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice adding a note about how it affects self.info.settings and self.info.options. Settings is None and settings.compiler is not make affect for check_min_cppstd, only when installing the package. Otherwise, it would require validate_build with self.settings.compiler

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should go into the main Conan docs since it's pretty generic.
"For more information about how this affects the package_id read the documentation here" type of thing?

@danimtb danimtb closed this Sep 20, 2022
@danimtb danimtb reopened this Sep 20, 2022
@conan-center-bot
Copy link
Collaborator

Updating docs!

@conan-center-bot conan-center-bot merged commit 383ed57 into conan-io:master Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants