This is the first CMP chunk with contributions also by @Akretsch.
He is going to be the main person handling review comments.

Thanks @mattcaswell and @FdaSilvaYY for your comments.
We have meanwhile handled all of them, performing all requested changes.

@mattcaswell, we have a general question regarding the review process of the CMP chunks.

Chunk 3 is still under review, and it seems likely that even if you approve it soon it will take some further while until it can be merged since a second review (which has not yet started) and approval is needed.
Chunk 4 will be rather orthogonal to chunk 3 (as it will focus on the OSSL_CMP_CTX structure with its getters and setters, plus some auxiliary and logging functions), and meanwhile we have got pretty far preparing it.

In order to save time (since the overall timeline for the CMP contribution until the release of 3.0.0 is pretty tough), would it make sense to submit chunk 4 somewhat in parallel to chunk 3, for example just after you have approved chunk 3?

Does chunk 4 depend on the chunk 3 commits? It's easier to review if it doesn't because github shows all code that isn't in the target branch when you are reviewing.

Unfortunately, there are a couple of files (at least cmp.h and cmp_int.h and some meta files) where there is a dependency because they get extended by each chunk.

Unfortunately, there are a couple of files (at least cmp.h and cmp_int.h and some meta files) where there is a dependency because they get extended by each chunk.

That is unfortunate. You could create chunk 4 as a WIP PR but github will show it with all the chunk 3 commits. It is possible to click into each individual commit and review each one in turn, but if there are multiple commits this is also problematic because it is difficult to get a "big picture" view.

Another workaround could be to submit at first only the 'meat' of the next chunk, i.e., the new source, doc, and test files, and to add the files that have dependencies later.

Unfortunately, there are a couple of files (at least cmp.h and cmp_int.h and some meta files) where there is a dependency because they get extended by each chunk.

That is unfortunate. You could create chunk 4 as a WIP PR but github will show it with all the chunk 3 commits. It is possible to click into each individual commit and review each one in turn, but if there are multiple commits this is also problematic because it is difficult to get a "big picture" view.

@DDvO you could open a pull request for chunk4 against chunk3 in your own repository for @mattcaswell to (p)review. Doing that, Matt would get to see the big picture, but without the commits from chunk3. To create the pull request, just go to the following comparison page:

https://github.com/mpeylo/cmpossl/compare/cmpossl_incremental3...mpeylo:cmpossl_incremental4?expand=1

(Currently, there seem to be merge conflicts, which you will need to fix first. Best is to rebase chunk4 on top of chunk3 first.)

You could even create two pull requests: an „official“ one against master in our repo, and an incremental one in your repo for reference.

@DDvO you could open a pull request for chunk4 against chunk3 in your own repository for @mattcaswell to (p)review. Doing that, Matt would get to see the big picture, but without the commits from chunk3.

This sounds like a good way to go, thanks @mspncp for this hint!
@mattcaswell, what do you think?

To create the pull request, just go to the following comparison page:
https://github.com/mpeylo/cmpossl/compare/cmpossl_incremental3...mpeylo:cmpossl_incremental4?expand=1

@mattcaswell, would you be able to give review comments in that repository?

(Currently, there seem to be merge conflicts, which you will need to fix first. Best is to rebase chunk4 on top of chunk3 first.)

Yes, this is because (as mentioned in a response to a review comment above: #8669 (comment)) I needed to squash (most of) cmpossl_incremental3. We'll rebase our preliminary cmpossl_incremental4 tomorrow.

You could even create two pull requests: an „official“ one against master in our repo, and an incremental one in your repo for reference.

@mspncp, I did not get what you mean here. Also the 'official' one is necessarily incremental.

You could even create two pull requests: an „official“ one against master in our repo, and an incremental one in your repo for reference.

@mspncp, I did not get what you mean here. Also the 'official' one is necessarily incremental.

The idea was that an 'inofficial' pull request in your clone would go unnoticed by most people except for Matt. However, an 'official' pull request against our repo would need to be made against master, so it would have to include the changes from chunk3. So the solution would be to make an official pull request against our master and in the description of the pr include a link to the relative pr on your repo. Whenever you push changes to chunk4, both pull requests would be updated.

Well, it's just an idea. You don't have to do it if it sounds too complicated.

(Note: I'm assuming that your branch for chunk4 will be based on top of the branch for chunk3.)

This sounds like a good way to go, thanks @mspncp for this hint!
@mattcaswell, what do you think?

Sounds like a good idea to me

@mattcaswell, I've just resolved all (new) review comments on this PR, handling all change requests.

This sounds like a good way to go, thanks @mspncp for this hint!
@mattcaswell, what do you think?

Sounds like a good idea to me

@mattcaswell, I've meanwhile rebased both branches, such that as mentioned by @mspncp :
https://github.com/mpeylo/cmpossl/compare/cmpossl_incremental3...mpeylo:cmpossl_incremental4?expand=1

you can have a clean preview of our chunk 4 and could already do a first review pass there.
What we have not yet done for chunk 4 is to add the respective portions of the documentation.

Btw: The cla bot is complaining about a missing CLA? Is it because of @Akretsch‘s commits?

Btw: The cla bot is complaining about a missing CLA? Is it because of @Akretsch‘s commits?

I was wondering about this CLA complaint, too. It is about @Rick113's commits.

I've just double-checked: Roman Schauer sent his CLA already on May 13th,
but apparently is has not yet made its way into the OpenSSL contributors' database.

I've just double-checked: Roman Schauer sent his CLA already on May 13th,
but apparently is has not yet made its way into the OpenSSL contributors' database.

cc @mattcaswell

Close/reopen to kick cla bot

I've just double-checked: Roman Schauer sent his CLA already on May 13th,
but apparently is has not yet made its way into the OpenSSL contributors' database.

Apologies for the delay on this. Roman's email ended up in my spam folder for some reason. All sorted now.

Squashed and pushed. Thanks all!