Closed hhugo closed 5 months ago
Ideally what I would like to do is to get good Windows support without making the codebase worse than it already is. I think that the current PR you propose is okay in this respect, the changes are relatively minimal and at the same time justified, and the risk of regression for non-Windows users is low. I propose to merge it and build up from there.
On the other hand I am more worried about the fdopen patches in #330, which look like a mix of good ideas (I like getting rid of the "or is win32" case in the "is degraded?" test), inscrutable hacks, and things in the middle, and was never really tested on non-Windows systems if I understand correctly. Taking the good pieces out of it is an excellent idea, and maybe all of it is good and I just don't understand it yet. But making our codebase harder to understand (obscure changes with little explanations and no commit message) is getting convenience right now but also even more pain later, so I would not be in favor.
Note: I noticed the same Be consistent!
comment in the present PR and in the .patch file somewhere. Are you the author of all the changes in the current PR? If not, are reused fragments properly credited and do they have a compatible software license?
https://github.com/ocaml/ocamlbuild/pull/329/commits/00b92ab0f67698e3d85be72f02c68d00a3374ec6 was extracted from an old PR #70 -> https://github.com/ocaml/ocamlbuild/pull/70/commits/62c40ca26173f794790243d608d6415d0a64a6ce because I was seeing the issue mentioned in this PR.
Could you cherry-pick that commit (or amend the author information on your own commit) to properly credit fdopen in the metadata?
Could you cherry-pick that commit (or amend the author information on your own commit) to properly credit fdopen in the metadata?
Done
@kit-ty-kate are you okay with merging this? My reasoning is: the patch looks okay, the potential for non-Windows regression is minimal, and I want to make it as easy as reasonably possible for @hhugo (or of course anyone else) to improve the state of ocamlbuild on Windows.
(@dbuenzli, just for information, do you have any plan to drop your dependency on ocamlbuild ?, any ETA ?)
I went ahead and merged. Thanks @hhugo!
(It may be possible to close the issues that you helpfully linked, but at least the most recent one had more changes that were more contentious, and might deserve a discussion of their own. I hope that the part that you included solves most of the issues with less risk of regression.)
any ETA ?
Not really, but I'd say within two or three years.
@kit-ty-kate are you okay with merging this? My reasoning is: the patch looks okay, the potential for non-Windows regression is minimal, and I want to make it as easy as reasonably possible for @hhugo (or of course anyone else) to improve the state of ocamlbuild on Windows.
Merging is fine as long as we remember to not release before getting the double-check/approval of the two people i've cc'd above. I'd like to especially double-check the change made to plugin-lib/ocamlbuild_unix_plugin.ml
and src/command.ml
, which seem a bit fragile, especially as:
bash
could be present in the environment, without it being linked to an MSYS2 or Cygwin installation (e.g. some programs like some git installation ship with bash.exe
)bash
uses --noprofile
in https://github.com/ocaml/ocamlbuild/pull/330, is there a reason why this has not been done here too?~ EDIT: nevermind i see why (-c
vs interactive)
This is a somewhat minimal list of changes to make the testsuite pass on windows. If you look at the CI jobs, you should see that the testsuite pass under windows.
Includes:
Probably fix #316