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

[sqlite-modern-cpp] update #37563

Merged
merged 6 commits into from
Mar 29, 2024

Conversation

darrenbane-fc
Copy link
Contributor

Updates the version of SqliteModernCpp/sqlite-modern-cpp to the latest commit, as the previous one was more than five years old

  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version.
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@darrenbane-fc
Copy link
Contributor Author

@microsoft-github-policy-service agree company="FileCloud"

@@ -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

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 :)

@MonicaLiu0311 MonicaLiu0311 changed the title Sqlite modern cpp update [sqlite-modern-cpp] update Mar 21, 2024
@MonicaLiu0311 MonicaLiu0311 added the category:port-update The issue is with a library, which is requesting update new revision label Mar 21, 2024
BillyONeal added a commit to BillyONeal/vcpkg-tool that referenced this pull request Mar 23, 2024
In microsoft/vcpkg#37563 (comment) we have an example of versions which meet the semantic versioning format but don't sort like semantic versions, so the message:

```console
PS D:\vcpkg> .\vcpkg.exe x-add-version sqlite-modern-cpp
Use the version scheme "version" rather than "version-string" in port "sqlite-modern-cpp".
Use --skip-version-format-check to disable this check.
```

is too strong. We need to explain to the user that they should only do that if the versions actually are intended to sort that way.
@BillyONeal
Copy link
Member

Just putting it on record that I think this is good to go subject to the versioning discussion above. Paths forward:

  • Change to version-string (my proposal)
  • Change to version-date (possible alternative suggested by @ras0219-msft )
  • Change to version but encode a date as the last number so that it's still orderable with current versions (that is, 3.2.20240326) (possible alternative suggested by @ras0219-msft )

@darrenbane-fc which of these do you think is least confusing?

@BillyONeal BillyONeal added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Mar 26, 2024
@darrenbane-fc
Copy link
Contributor Author

I agree that upstream seem to have stopped making full releases and expect to work off HEAD. I'll raise a ticket there just in case, maybe they would create a 3.3 tag, but I don't think it's a good idea to depend on this.

I think version-string will be easiest in future. IMHO a git commit hash is more definitive than a date/time where something has to map the date/time to a commit hash anyway. You lose the ordering, but I didn't even know that was a thing at the start, and version-string is like a hint that ordering is not a valid op now anyway?

So in conclusion I'd favour version-string, but like I said I'm not a vcpkg expert.

@BillyONeal
Copy link
Member

@vicroms version-string is kind of 'user experience hostile' so I prefer that we move away from it (and I prefer version-date)
@dan-shaw I agree with @vicroms
@ras0219-msft I agree with @vicroms but I agree with him more strongly than he put it. I believe this is the purpose of version-date and I think we should use it for all of these sort of rolling release cases. Importantly, it plays well with upstreams who decide to change to having real versions and back.

Result: @BillyONeal to change this to version-date and merge.

@BillyONeal BillyONeal added info:reviewed Pull Request changes follow basic guidelines and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Mar 29, 2024
@BillyONeal BillyONeal merged commit 95bfe3a into microsoft:master Mar 29, 2024
16 checks passed
@BillyONeal
Copy link
Member

Thanks for the update @darrenbane-fc and sorry it took a bit for us to get the versioning in a good place :)

BillyONeal added a commit to microsoft/vcpkg-tool that referenced this pull request Apr 29, 2024
In microsoft/vcpkg#37563 (comment) we have an example of versions which meet the semantic versioning format but don't sort like semantic versions, so the message:

```console
PS D:\vcpkg> .\vcpkg.exe x-add-version sqlite-modern-cpp
Use the version scheme "version" rather than "version-string" in port "sqlite-modern-cpp".
Use --skip-version-format-check to disable this check.
```

is too strong. We need to explain to the user that they should only do that if the versions actually are intended to sort that way.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants