Conversation
Signed-off-by: Ali Caglayan <alizter@gmail.com>
| /* Clearing readers is done in this function because | ||
| * me_txkey with its destructor must be disabled first. | ||
| * | ||
| * We skip the reader mutex, so we touch only |
There was a problem hiding this comment.
This was removed by @ElectreAAS at some point, but since it is vendored code it has come back FYI.
There was a problem hiding this comment.
How did such a change go through code review?
There was a problem hiding this comment.
That's my mistake, I missed it. My solution for #12457 should prevent this from happening in the future.
There was a problem hiding this comment.
The PR in question was #12805 and the change was at the very bottom of the changes list and I missed it. It's the only file I didn't mark as viewed.
There was a problem hiding this comment.
If these files are meant to always and only be generated by the scripts like https://github.com/ocaml/dune/pull/12926/changes#diff-25acefe0cd708e88cfffb9dcdec1c96e363ad79ed70545c93892746f53684267 (or ocaml alternates) we could add a CI check that runs the scripts and fails if the files change.
There was a problem hiding this comment.
Yes, my solution for #12457 will add a way of verifying this in CI, when I get round to it.
This prevents the build of Dune on FreeBSD which is something that was possible before. This is completely unrelated to #12896. It is not critical that we block the release on this, what I am suggesting above is that if we do a patch release, i.e. 3.21.1 then it might be worth including otherwise waiting for 3.22 is also fine. |
|
I've created an issue for it in #12927. It is technically a regression introduced in this release. |
|
Got it. Sorry for the confusion, thank you for giving enough context so that the developments here are a bit more understandable :) |
| #elif defined(__FreeBSD__) | ||
| #include <stdlib.h> |
There was a problem hiding this comment.
This appears to be a new addition, and not just reverting the error from. https://github.com/ocaml/dune/pull/12805/changes#diff-54c0e4bcfcbcab9d492b3d752917da5bdd85283dc62b431124dfaaafa8b789b2 ? Is that correct? What motivates its addition here in the regression fix ?
There was a problem hiding this comment.
Ah, I think i figured out, it must be additions from rerunning the vendor script?
There was a problem hiding this comment.
Exactly. The vendor script is the only thing that should be changed.
| /* Clearing readers is done in this function because | ||
| * me_txkey with its destructor must be disabled first. | ||
| * | ||
| * We skip the reader mutex, so we touch only |
There was a problem hiding this comment.
If these files are meant to always and only be generated by the scripts like https://github.com/ocaml/dune/pull/12926/changes#diff-25acefe0cd708e88cfffb9dcdec1c96e363ad79ed70545c93892746f53684267 (or ocaml alternates) we could add a CI check that runs the scripts and fails if the files change.
The lmdb stubs needed some tweaking. This should allow the builds on FreeBSD in the opam ci to progress.
@shonfeder this shouldn't block the release but probably nice to have in a patch release for 3.21.