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

tools.scm.Version in develop as is in develop2 #11823

Merged

Conversation

lasote
Copy link
Contributor

@lasote lasote commented Aug 9, 2022

Changelog: Bugfix: The tool.scm.Version model has been ported from 2.X to keep the same behavior in Conan 1.X.
Docs: omit

Close #11821

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

I guess it is ok, and necessary for the migration.

@lasote lasote marked this pull request as ready for review August 10, 2022 06:13
@lasote lasote added this to the 1.52 milestone Aug 10, 2022


@total_ordering
class Version:
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add as_list, so it could be minimal compatible with what we have in Conan Center Index now.

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't like that, I think that as_list is an ugly interface that if we do that it will "infect" Conan 2.0.
If necessary, please let us know the use cases of such method, and we can think about what is the right interface for it.

Copy link
Member

Choose a reason for hiding this comment

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

I still prefer the legacy, as described in this PR. The only thing is re-editing recipes again in CCI. Also, we will need to create some if conan_version else because the different behavior from 1.49 (i guess) til 1.52

Copy link
Member

Choose a reason for hiding this comment

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

I haven't found occurrences of as_list in conan-center-index (only one method in openssl recipe), am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

Boost recipe: https://github.com/conan-io/conan-center-index/blob/master/recipes/boost/all/conanfile.py#L1461

If you are running GH search, forget, it's useless most of time.

Copy link
Member

Choose a reason for hiding this comment

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

Furthermore, as_list is not a documented/public method at all, shouldn't be used in any of the Version implementations? While major and minor are. Not sure why they used that in the boost recipe.

Copy link
Member

Choose a reason for hiding this comment

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

The problem was, Boost recipe need to compare some major and minor versions, but the new scm.Version does not support semver, so it return "1.2" for Version.minor(fill=False). Of course, the previous behavior, with semver, was much better. I gonna revert those changes and keep aligned with this new PR. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you mean. This new Version in this PR supports semver like comparison between versions, no problem with that. How is that related to the as_list usage, which is done to just extract the major, minor and patch to compose some strings (suffixes) in the boost recipe?

Copy link
Member

Choose a reason for hiding this comment

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

  • The new Version changed major, minor and patch from property to method
  • When you call conan.tools.scm.Version('1.2.3').minor() it returns 1.2.Z
  • When you call conan.tools.scm.Version('1.2.3').minor(fill=False) it returns 1.2
  • But still, I only want the minor, the 2.
  • As alternative, I can consume from conan.tools.scm.Version('1.2.3')[1]

Anyway, this new PR fixes the case, so we can use Version.major as a property again.

Copy link
Member

Choose a reason for hiding this comment

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

When I said The new Version, I meant which is production right now and running on CCI, not this PR. This PR is correct, it follows the previous and expected behavior.

@czoido czoido merged commit a9a2c2e into conan-io:develop Aug 10, 2022
czoido pushed a commit to czoido/conan that referenced this pull request Aug 10, 2022
czoido pushed a commit to czoido/conan that referenced this pull request Aug 10, 2022
czoido added a commit that referenced this pull request Aug 10, 2022
SpaceIm added a commit to SpaceIm/conan-center-index that referenced this pull request Aug 10, 2022
conan-center-bot pushed a commit to conan-io/conan-center-index that referenced this pull request Aug 16, 2022
* conan v2 support

* fix injection of bzip2 major version

* use conan.tools.scm.Version again

* no conan.tools.scm.Version to get major bzip2 version

wait conan-io/conan#11823
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature] Conan tools.scm.Version became very difficult to use
4 participants