vendor: ocaml vendoring utility - #13097
Conversation
8464cef to
750cbe3
Compare
Signed-off-by: Ali Caglayan <alizter@gmail.com>
750cbe3 to
54e4be2
Compare
| $ dune exec vendor/vendor_updater.exe -- patch cmdliner | ||
|
|
||
| # Validate that all patches are up to date | ||
| $ dune exec vendor/vendor_updater.exe -- lint |
There was a problem hiding this comment.
How about we make check this a rule in vendor that is attached to runtest?
|
|
||
| #if defined(_MSC_VER) | ||
| #include <windows.h> | ||
| #include <Windows.h> |
There was a problem hiding this comment.
Was changed in #11940 but not upstream. So this pulls it back in line. The Microsoft sources ship uppercase however according to this https://stackoverflow.com/a/15466951/2604678 mingw ships windows.h (CI doesn't seem to mind). Windows is case insensitive but cross-compiling with mingw shows this problem.
There was a problem hiding this comment.
We need to update upstream rather than doing this change here. Alternatively we could just keep a patch around using the new mechanism here.
There was a problem hiding this comment.
Yeah, let's move out this change out of here. #11940 should have introduced a patch file
| Dune_console.Backend.set Dune_console.Backend.progress | ||
| ;; | ||
|
|
||
| let () = |
There was a problem hiding this comment.
All of this code should be moved outside of vendor/. This directory is reserved for the actual stuff that we vendor rather than our utility that we use to manage this code.
There was a problem hiding this comment.
My original plan was to put it in a subdirectory of vendor but (vendored_dirs) cannot do set minus.
There was a problem hiding this comment.
I will move it to a tools/ directory of the main repo.
| ; preserve_rules : string list | ||
| } | ||
|
|
||
| let all_packages = |
There was a problem hiding this comment.
Could you move this binding to a separate module that doesn't have any other code or type definitions in it. Would be good to have the file that we actually need to edit be as pristine as possible.
There was a problem hiding this comment.
I will introduce a sexp-based config file for it to read in the vendor/ directory.
I want to also introduce some cram tests to test the various behaviours we have. Just having it in the repo doesn't mean we didn't break it. Having the extra config file will aid testing.
There was a problem hiding this comment.
Please don't do that. It seems like a poor trade-off to introduce a hefty amount of sexp conversions just to make it easy to write cram tests. Write a different type of tests.
There was a problem hiding this comment.
What about having both allowing us to override the packages in the tests. That way we don't incur the cost of sexp in normal usage and it is easy to test.
There was a problem hiding this comment.
You know what, I'll try both expect tests and cram tests in parallel and see which one ends up being better.
There was a problem hiding this comment.
What about having both allowing us to override the packages in the tests. That way we don't incur the cost of sexp in normal usage and it is easy to test.
Why is that you need sexps to create mock data for your utility? I can see many different ways to mock this data without any sexps.
You know what, I'll try both expect tests and cram tests in parallel and see which one ends up being better.
Whatever you go for, please don't introduce sexp deoders.
| (* Initialize Path module and console backend *) | ||
| let () = | ||
| (* Find project root by looking for dune-project *) | ||
| let rec find_up dir = |
There was a problem hiding this comment.
Why can't we use our existing root finding?
There was a problem hiding this comment.
It's copy-pasted since I didn't want to move it out of bin. I can put it in dune_util however for reuse.
|
|
||
| .PHONY: vendor-lint | ||
| vendor-lint: $(BIN) | ||
| @$(BIN) exec -- vendor/vendor_updater.exe lint |
There was a problem hiding this comment.
As mentioned earlier, these checks should be defined as part of the runtest alias.
| Console.printf "Patch applied successfully") | ||
| else | ||
| let+ () = Fiber.return () in | ||
| Console.printf "No patch file found for %s" package_name |
There was a problem hiding this comment.
I think this message is just noise
There was a problem hiding this comment.
I will double check how it is printed when testing.
|
This would have been nice, but the current implementation has some problems which I didn't get round to fixing. For now we are stuck with the status quo until sombebody decides its important enough to revive. |
2 participants
desc todo