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.

Sign up for GitHub

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

[sqlite-modern-cpp] update #37563

Merged
merged 6 commits into from
Mar 29, 2024
Merged

[sqlite-modern-cpp] update #37563

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3fb6382
update version of sqlite-modern-cpp
darrenbane-fc Mar 20, 2024
f44a222
vcpkg x-add-version
darrenbane-fc Mar 20, 2024
71096f6
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal Mar 23, 2024
1b89325
Change to version-string.
BillyONeal Mar 23, 2024
65274fa
Merge remote-tracking branch 'origin/master' into HEAD
BillyONeal Mar 29, 2024
cb4f38b
Change to version-date
BillyONeal Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions ports/sqlite-modern-cpp/portfile.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,8 @@
vcpkg_from_github(
OUT_SOURCE_PATH SOURCE_PATH
REPO SqliteModernCpp/sqlite_modern_cpp
REF 936cd0c86aacac87a3dab32906397944ae5f6c3d
SHA512 8ce1b7593fe77dcab297ab4cae0158b43d55b33c1823b2dc5bf22e5545d9781d675ba5ac82b81782f502b34d2335eee2c26167726746a61a0ad566b657d2faf0
REF 6e3009973025e0016d5573529067714201338c80
SHA512 a007c739e00b9bd51d19f3bc484709f9fc4637f0262b636b51ee95cbc7f3f7fe6551dcbf0990a0430ac12f276bb72d1e0a3b71f06ac6e6d19fb46d51066a4295
HEAD_REF master
)

Expand Down
3 changes: 1 addition & 2 deletions ports/sqlite-modern-cpp/vcpkg.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{
"name": "sqlite-modern-cpp",
"version": "3.2-936cd0c8",
"port-version": 2,
"version": "3.2-6e300997",
Copy link
Member

Choose a reason for hiding this comment

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

This is a problem because the new version does not sort after the old version. (That is, 3.2-936cd0c8#2 > 3.2-6e300997#0)

See https://semver.org/#spec-item-11

Should this port use version-string instead?

Copy link
Contributor

@dg0yt dg0yt Mar 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@BillyONeal what keeps us from using + (instead of -) AND increasing the port number?

(None of the 3.2-xxx versions is sorted after 3.2.)

Copy link
Member

Choose a reason for hiding this comment

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

@BillyONeal what keeps us from using + (instead of -) AND increasing the port number?

Nothing technically although there's no benefit to doing so over using version-string at that point, since any conflicts of build metadata must be resolved with overrides.

(None of the 3.2-xxx versions is sorted after 3.2.)

Yikes! 😭😭😭😭😭

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought there is a benefit, and no conflict, when version 3.2 and 3.2-foo are equal, but 3.2-foo carries the higher port-version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'm a first-time contributor, but is there a concrete change you require before merge? From reading the semver spec it looks like '+' should have been used, but it's certainly a bad idea to change historic versions (that people might depend on). Should I change this 6e30... update only to use '+'? This means it will sort at the same position as "3.2", is that ok? I'm not quite sure why sorting is so important here, I thought real usage of vcpkg would lock to a particular dependency version.

Also, from my reading I don't particularly see a benefit of using "version-string", the above behaviour for "version" seems reasonable?

Finally, I can increment port-version to "3" instead of resetting it back to 0, but I thought that it was supposed to reset when "version" changed, i.e. any sorting by port-version was within the individual "version". Is this correct?

Thanks for the help. I thought I was doing the right thing by copying what was there before, but it seems there are some problems there.

Copy link
Member

Choose a reason for hiding this comment

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

I thought there is a benefit, and no conflict, when version 3.2 and 3.2-foo are equal, but 3.2-foo carries the higher port-version.

No, 3.2 > 3.2-anything. Sort order of what version says happens before port-version.

From reading the semver spec it looks like '+' should have been used

  • isn't quite right either because this isn't really a build metadata change

Also, from my reading I don't particularly see a benefit of using "version-string", the above behaviour for "version" seems reasonable?

The point of version-string rather than version here is that it tells vcpkg that the versions are not orderable, and that if a conflict is encountered users need to resolve the conflict with overrides.

Consider Port A:

{
    "dependencies": [{
        "name": "sqlite-modern-cpp",
        "version>=": "3.2"
     }]
}

and B:

{
    "dependencies": [{
        "name": "sqlite-modern-cpp",
        "version>=": "3.2-936cd0c8"
     }]
}

and C:

{
    "dependencies": [{
        "name": "sqlite-modern-cpp",
        "version>=": "3.2-6e300997"
     }]
}

and vcpkg install A B C, vcpkg will choose 3.2 (no suffix). "version" tells vcpkg "you can order these according to semver rules", and semver rules say "anything with a - sorts before anything without" and "versions which both have - sort the part after the - lexicographically", and 9 > 6. So vcpkg currently thinks the order is:

  • 3.2 (newest)
  • 3.2-936cd0c8 (newest - 1)
  • 3.2-6e300997 (newest - 2)

but the port author wanted the order to be:

  • 3.2-6e300997 (newest)
  • 3.2-936cd0c8 (newest - 1)
  • 3.2 (newest - 2)

Using "version-string" would instead tell vcpkg "you should not try to order these versions", and given vcpkg install A B C it would fail and prompt the user to pick the version they want.

Finally, I can increment port-version to "3" instead of resetting it back to 0, but I thought that it was supposed to reset when "version" changed, i.e. any sorting by port-version was within the individual "version". Is this correct?

Your resetting of port-version when changing version is correct here. The user is getting different sources so the version should change, not only the port-version.

Thanks for the help. I thought I was doing the right thing by copying what was there before, but it seems there are some problems there.

Yeah, sorry this was missed before :/

Copy link
Member

Choose a reason for hiding this comment

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

I tried to make our explanation from the tool better over here: microsoft/vcpkg-tool#1367

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, after reading that I understand why "version-string" is what should be used here. And I see you changed that now.

I think that's as good as we can do then, is it? Is there anything else that would block a merge?

Copy link
Member

Choose a reason for hiding this comment

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

I think that's as good as we can do then, is it? Is there anything else that would block a merge?

Alternately, it looks like upstream has decided to stop doing releases; 3.2 is from 2017.

As a customer of this thing, would you find switching to version-date entirely reasonable?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, sorry this was missed before :/

In fairness 2018 ( 80f64c2 ) was a different time long before we had strong versioning support :)

"description": "The C++14 wrapper around sqlite library",
"homepage": "https://github.com/aminroosta/sqlite_modern_cpp",
"dependencies": [
Expand Down
4 changes: 2 additions & 2 deletions versions/baseline.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8301,8 +8301,8 @@
"port-version": 0
},
"sqlite-modern-cpp": {
"baseline": "3.2-936cd0c8",
"port-version": 2
"baseline": "3.2-6e300997",
"port-version": 0
},
"sqlite-orm": {
"baseline": "1.8.2",
Expand Down
5 changes: 5 additions & 0 deletions versions/s-/sqlite-modern-cpp.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "22557f78d41d269880dc04ef83c1d4147e8f7a84",
"version": "3.2-6e300997",
"port-version": 0
},
{
"git-tree": "b468e28a38d1849eecaa0113410b214dbe41cd27",
"version": "3.2-936cd0c8",
Expand Down
Loading