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

api: correct DC_CERTCK_ACCEPT_* values and docs #6176

Merged
merged 3 commits into from
Nov 13, 2024

Conversation

r10s
Copy link
Member

@r10s r10s commented Nov 4, 2024

this PR changes DC_CERTCK_ACCEPT_* to the same values in cffi as rust does. and regards the same values as deprecated afterwards

there is some confusion about what is deprecated and what not, see deltachat/deltachat-android#3408

iOS needs to be adapted as it was following the docs in the CFFI before, same desktop. both need to be graceful on reading and strict on writing.

this PR is considered harmful, so we should not merge that in during 1.48 release, there is no urgency, things are fine (wondering if it isn't even worth the effort, however, having different values and deprecations is a call for trouble in the future ...)

@r10s r10s requested review from Hocuri and link2xt November 4, 2024 17:13
@r10s

This comment was marked as off-topic.

@adbenitez

This comment was marked as off-topic.

@r10s

This comment was marked as off-topic.

@Hocuri Hocuri force-pushed the r10s/fix-cert-checks-defines branch from f958376 to 8b15c14 Compare November 4, 2024 19:11
@Hocuri

This comment was marked as off-topic.

@link2xt
Copy link
Collaborator

link2xt commented Nov 5, 2024

Still need to run npm run build:core:constants.

@r10s
Copy link
Member Author

r10s commented Nov 5, 2024

Still need to run npm run build:core:constants

did so.

this PR is considered harmful

we should not merge that in during 1.48 release phase, the changed defines may easily break things, iOS and desktop need to accept both of DC_CERTCK_ACCEPT_INVALID_CERTIFICATES and DC_CERTCK_ACCEPT_INVALID_CERTIFICATES2 EDIT: disarmed, see below

@r10s r10s changed the title correct DC_CERTCK_ACCEPT_* values and docs [do not merge while 1.48 releases] correct DC_CERTCK_ACCEPT_* values and docs Nov 5, 2024
@r10s r10s force-pushed the r10s/fix-cert-checks-defines branch from 65856df to 1a85b7c Compare November 11, 2024 20:09
@r10s r10s changed the title [do not merge while 1.48 releases] correct DC_CERTCK_ACCEPT_* values and docs correct DC_CERTCK_ACCEPT_* values and docs Nov 11, 2024
@r10s
Copy link
Member Author

r10s commented Nov 11, 2024

k, i disarmed the potentially harmful part of this PR: existing constant stay unchanged, only the missing one is added. this only leads to some weird names :)

if implementation do nothing, things stay as before, and things can be improved without stress, without generating blocking issues.

when this PR is merged, we should file issues on desktop/iOS that only DC_CERTCK_ACCEPT_INVALID must be written and DC_CERTCK_ACCEPT_INVALID as well as DC_CERTCK_ACCEPT_INVALID_CERTIFICATE must be accepted on reading 1

Footnotes

  1. that could have been fixed also by a migration or maybe by not changing the constant in the API beforehand, but now it's like that and probably the easiest way, also as the values are gapless now, which is nice)

@r10s r10s requested a review from Hocuri November 11, 2024 23:30
deltachat-ffi/deltachat.h Outdated Show resolved Hide resolved
Co-authored-by: iequidoo <117991069+iequidoo@users.noreply.github.com>
@r10s r10s changed the title correct DC_CERTCK_ACCEPT_* values and docs api: correct DC_CERTCK_ACCEPT_* values and docs Nov 13, 2024
@r10s r10s enabled auto-merge (squash) November 13, 2024 16:41
@r10s r10s merged commit 010b655 into main Nov 13, 2024
37 checks passed
@r10s r10s deleted the r10s/fix-cert-checks-defines branch November 13, 2024 16:46
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.

5 participants