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

b2: update 'standard' recipe to match the newer 'portable' #18089

Closed
wants to merge 2 commits into from

Conversation

valgur
Copy link
Contributor

@valgur valgur commented Jun 27, 2023

TODO

  • Empty package

@github-actions
Copy link
Contributor

🤖 Beep Boop! This pull request is making changes to 'recipes/b2//'.

👋 @grafikrobot you might be interested. 😉

@ghost
Copy link

ghost commented Jun 27, 2023

I detected other pull requests that are modifying b2/portable recipe:

This message is automatically generated by https://github.com/ericLemanissier/conan-center-conflicting-prs so don't hesitate to report issues/improvements there.

@conan-center-bot

This comment has been minimized.

Copy link
Contributor

@grafikrobot grafikrobot left a comment

Choose a reason for hiding this comment

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

AFAICT the changes you made are:

  1. A bunch of style only changes. Why?
  2. Adding compiler and build_type to settings. Why?
  3. Using option_descriptions. Great!
  4. Clearing of some cpp_info vars. Great!
  5. Putting the old and new build code into one recipe by adding the _build_non_portable function and version check. Why?

I'm wondering why you did (1) as it just makes the history harder to follow.

I'm wondering why you did (2). As mentioned in the other comment.. This goes against an intentional choice.

Last, (5). I really don't understand the goal of this PR. Why complicate the new build and install mode with the old mode? Is there some use case or problem you are trying to fix?

recipes/b2/all/conanfile.py Outdated Show resolved Hide resolved
@valgur valgur changed the title b2: merge 'standard' and 'portable' recipes b2: update 'standard' recipe to match the newer 'portable' Jun 28, 2023
@valgur
Copy link
Contributor Author

valgur commented Jun 28, 2023

  1. A bunch of style only changes. Why?

Sorry, these were incidental and not really relevant. Reverted them.

  1. Adding compiler and build_type to settings. Why?

Added these to match the current recommendation to always list all values and then remove the irrelevant settings in package_id(): https://github.com/conan-io/conan-center-index/blob/master/docs/adding_packages/conanfile_attributes.md#settings

  1. Putting the old and new build code into one recipe by adding the _build_non_portable function and version check. Why?
    Last, (5). I really don't understand the goal of this PR. Why complicate the new build and install mode with the old mode? Is there some use case or problem you are trying to fix?

The intention behind this PR was (1) to eliminate unnecessary dead code in the repo by updating the standard recipe and really (2) to personally get some practice on how the environment variables should be handled in Conan v2 before starting work on migrating some of the more complex Autotools recipes.

I originally did not intend to modify the portable recipe at all, but the differences between the two recipes appeared to be quite small, so it made sense to merge them instead.

Anyway, I reverted the merge of the standard and portable recipes now and only kept minimal changes to the portable one. The standard recipe is now identical to portable with only the build() method differing. A minor benefit of keeping them separate is reducing the number of versions to build when the newer one gets updated, I suppose, since the list of versions is quite long.

@conan-center-bot

This comment has been minimized.

@ghost ghost mentioned this pull request Jul 11, 2023
3 tasks
@conan-center-bot

This comment has been minimized.

@conan-center-bot

This comment has been minimized.

1 similar comment
@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ❌

Sorry, the system is under maintenance and it doesn't accept builds right now.
Please, check https://status.conan.io to obtain more information.
Thanks for your understanding and help with the Conan Center Index!


Conan v2 pipeline ❌

Note: Conan v2 builds may be required once they are on the v2 ready list

The v2 pipeline failed. Please, review the errors and note this will be required for pull requests to be merged in the near future.

See details:

Sorry, the system is under maintenance and it doesn't accept builds right now.
Please, check https://status.conan.io to obtain more information.
Thanks for your understanding and help with the Conan Center Index!

@valgur
Copy link
Contributor Author

valgur commented Jul 26, 2023

Closing temporarily to avoid unnecessary load on the CI. Will reopen when I'm actively working on the PR again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants