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

adding rapidjson/20200410 #1464

Merged
merged 4 commits into from
May 12, 2020
Merged

Conversation

prince-chrismc
Copy link
Contributor

@prince-chrismc prince-chrismc commented Apr 26, 2020

Specify library name and version: rapidjson/20200410

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

There's a bug in 1.1.0 that has been fixed by upstream regarding patternProperties not correctly validating more than once when they contain properties.

Comment on lines 1 to 5
versions:
"1.1.0":
folder: "1.1.x"
"20200410":
folder: "all"
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 am mixing tagged release with using master, it's been 4 years since the last release and 500+ commits.

is this the correct approach? It's 99% copy paste however.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah there's a ton of issues asking for one lol

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like mixing at all.

How much work would it be to backport this one bug fix?

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 don't either...

A lot. I couldn't find an issue, and I'm not sure what code was broken in the first place. Updated my local copy to master and the crash went away. The issue and commit might have been written in chinses making it harder to track down.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't know when the next tag of rapidjson will happen

I reached out to find out, it will be very unlikely that there will ever be a new release.

we don't know if any of those 500+ commits has broken the API

There's a few API changes, I needed to modify my code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can not mix numeric release versions and a date version. ref So we are stuck taking something that respects semVer...

After more digging into this project there is a v1.2 target which was presented

Since this is a "future release" could we then use 1.2.0-cci.20200410

There was concerns about "guessing" the next release number but this is the official word from the main developer done here just after this comment

@jgsogo @Croydon

Would 1.2.0-cci.20200410 be a suitable compromise?

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be somewhat okay-ish for me in this case, but I don't think that it is a solution that scales for all projects which might be in a similar situation.

While it looks pretty safe for rapidjson that the next version is 1.2, plans can change at any time and we can't really de-publish any version.

What if rapidjson (or any other recipe/project in a similar situation) decides that next version is actually named 1.1.1 after we published 1.2.0-cci.something? People using version ranges would then get an older AND unofficial version rather than the latest official version.

I think that this is much, much worse than consciously breaking the semantic meaning of semver for an unofficial version, which should probably only be explicitly used anyway.

If we use 1.1.0-cci.20200410 the people using version ranges won't get this unofficial release automatically, but rather the official release 1.1.0, which might be favourable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about a separate projects?

  • rapidjson/1.1.0
  • rapidjson-master/20200410

There's no mixing and everything follows the current model, with a slight twist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is using build identifiers out of the question?
something like: rapidjson/1.1.0+cci20200410

@conan-center-bot
Copy link
Collaborator

All green in build 1 (8f26fa760ce739c5cf71aa358077552e04176ddf)! 😊

@@ -1,3 +1,5 @@
versions:
"1.1.0":
folder: "1.1.x"
"20200410":
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"20200410":
"1.1.0-next.0":

Adding a -identifier states in semver that it is a pre-release
next because it should communicate that it is the next version after 1.1.0
Adding a dot-separated pre-release identifier starting counting at 0 for the case that we need newer 1.1.0-next versions based on different project revisions for the case that the project still won't make a new release


<valid semver> ::= <version core> "-" <pre-release>

<pre-release> ::= <dot-separated pre-release identifiers>

<dot-separated pre-release identifiers> ::= <pre-release identifier>
                                          | <pre-release identifier> "." <dot-separated pre-release identifiers>

<pre-release identifier> ::= <alphanumeric identifier>
                           | <numeric identifier>

https://semver.org

prince-chrismc and others added 2 commits April 27, 2020 08:46
…emVer syntax

the semantics are "latest upstream tagged release" -cci. "commit date"

Co-Authored-By: Anonymous Maarten <madebr@users.noreply.github.com>
Co-Authored-By: Michael "Croydon" Keck <Croydon@users.noreply.github.com>
@conan-center-bot
Copy link
Collaborator

All green in build 2 (dce7099525b41b28db43ebb61f767a949245c328)! 😊

uilianries
uilianries previously approved these changes Apr 28, 2020
SSE4
SSE4 previously approved these changes Apr 29, 2020
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.

I understand we need (and we can) provide a version on our own, and these libraries that are waiting for a new release so long could benefit from the flexibility that Conan Center provides.

Some comments:

  • it cannot be a prerelease according to SemVer, as it would be previous to the released one, so the syntax 1.1.0- won't work.
  • probably we cannot assume that this commit will be before any 1.1.1 or x.y.z release. The author can decide to create a tag from a previous commit.

There is a common problem here: how to "tag" versions from commits in the repo, we need some consistency here. What are we doing on other recipes? Just the date?

@prince-chrismc
Copy link
Contributor Author

Yeah this is a complicated topic.

There is a common problem here: how to "tag" versions from commits in the repo, we need some consistency here. What are we doing on other recipes? Just the date?

Just the date.

However, to my knowledge, there is no recipe that mixes tagged releases and commit/date versioning. So this will be an exception by all means.

When Tencent acquired the RapidJSON library they needed to change the licensing under their own name. The consequence of this is any release would be their responsibility as such they are very admit on never making a release.

I have come around to the idea of using the pre-release semantics because it is "less then" the official tagged release which preceded it. Which shows it should not be weighted as highly as the official version.

There was a lot of great comments in the first bubble. One which stood out was with a number and date it's impossible to know the order.

@danimtb
Copy link
Member

danimtb commented May 11, 2020

We have been discussing this internally and here is the rationale about the different possibilities for the versioning issue:

  • We don't want to create a new artificial version that could be confused with a real tagged version released by the author.
  • It should be easy to identify it chronologically.
  • It should be not semver valid to avoid collision with version rages. If you want to use this version, declare it explicitly.

Proposals:

  • 1.1.0-<date>: Can be identified as a pre-release and it does not make sense in this context. 👎
  • 1.1.0-cci.<date>: Can be identified as a pre-release as well 👎
  • cci.<date>: Does not comply to semver 👍
  • 20200410: Is considered semver valid and would be identified as > 1.1.0 in a version range 👎
  • 2020-04-10: Semver compatible as pre-release and loose 👎
  • 1.1.0+zdsfsdfs: Using metadata is the same as not using it in semver 👎
  • snapshot.<date>: A bit long ❔
  • snap.20200410: Could be misleading regarding the snap package manager 👎
  • date.20200410: Iteration over the above ❔
  • rapidjson.20200410: Could be, but we would like to have a pattern valid for other packages with a similar situation 👎

We like cci.<date> (cci.20200410) as it is not semver compliant, is something that can be tracked in time and the cci prefix indicates that it is a version created on purpose by conan-center-index.

Please share your thoughts about this and let's get it documented in the wiki for other contributors! Thanks 😄


Other funny alternatives 🤣

donotuse.20200410
dontuse.rapidjson.author.wont.tag
goodluck.20200410

@prince-chrismc
Copy link
Contributor Author

goodluck.20200410

Would absolutely be perfect As my next commit message xD 🤣

I am a huge fan of cci.<date> I think it conveys the right message and implies what it is. Makes our lives easy to work with and easy for the consumer to understand when they are search for a version of a package!

I'll fix this shortly.

@prince-chrismc
Copy link
Contributor Author

prince-chrismc commented May 11, 2020

ERROR: rapidjson/cci.20200410: Error in source() method, line 21
        version = tools.Version(self.version)
        ConanException: Invalid version 'cci.20200410'

The only issue with the new version is that is can not be parsed by the tool

@prince-chrismc prince-chrismc dismissed stale reviews from SSE4 and uilianries via fd01761 May 11, 2020 21:30
@conan-center-bot
Copy link
Collaborator

All green in build 3 (fd01761503abb42f74b0e5a2763357adfd29da42)! 😊

@prince-chrismc prince-chrismc requested review from uilianries and SSE4 May 11, 2020 23:10
@Croydon
Copy link
Contributor

Croydon commented May 12, 2020

cci.<date> LGTM for versions we release on our own for projects which have done releases at some point, but not in a long time

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.

10 participants