ocamlbuild: add patch to work on windows from fdopen #23832
Conversation
|
I think it would still be nicer to apply fixes upstream instead of patching opam-repository. (This would help people that are not using opam-repository for example; I could see Diskuv building things from source with pre-cooked recipes.) Some quick remarks:
|
|
Thanks for the rapid review @gasche. Note that this PR does not affect non-Windows installations, as the patch is only applied if I'll defer your comments on upstreaming the patch for the purposes of merging this PR, as it's important that we unblock the heart of the dependency graph in opam-repo. I do hope @MisterDA or others can shepherd the patch upstream, as it hasn't aged quite as gracefully after lingering for 7 years as a nice glass of scotch might. @patricoferris might you be able to confirm that this PR removes the need for pin-depends in eio/windows for you? |
It pains me somewhat (!!), but I think this reason is indeed the argument for making an exception solely for ocamlbuild and incorporating this patch here for the time being. I also agree with @gasche that there are aspects of the patch which are "scary" in terms of blindly upstreaming. It would of course be good get upstream to a state where the patch isn't needed. That said, it's slightly painful to prioritise it given both ocamlbuild's largely flawed architecture where Windows is concerned (it's quite heavily Cygwin-reliant compared to Dune - I can't remember what the state of its parallel engine is) and its somewhat reduced usage these days. |
|
I feel that I am missing context to participate to the discussion. From my uninformed perspective, it is hard to guess why a patch that nobody cared about for 10 years suddenly needs to be urgently applied at the opam-repository level to get the property, if I understand correctly, that all the vendored patches that eio relies on are instead merged upstream (for the clean ones, I suppose) or vendored directly in the opam repository (for the ugly ones). If at some point someone is interested in upstreaming these changes in ocamlbuild, I think that a nice first step would be to re-split this patch into a series of independent commits, with commit messages explaining what the changes are doing. (Some of the changes here are also included in ocaml/ocamlbuild#70, and there the PR message has some explanations that could be reused.) |
|
@gasche wrote:
Because:
I've long been a proponent of putting patches like this into opam-repo with appropriate constraints so that they do not affect mainline users, yet help us advance the upstreaming process. In this case, as @dra27 notes, this patch allows other Windows development to happen in parallel while its own upstreaming process continues. |
|
I'll try to answer a few of these remarks as best I can, note I'm very much not a Windows programmer. Firstly, @gasche you are quite right that the pinned ocamlbuild in my fork and the original PR are not identical. I tried to make this clear in the Eio PR saying "which I think is the essence of this PR ocaml/ocamlbuild#70" -- apologies for the confusion here.
As far as I could tell (I definitely tried both), the PR does not fix ocamlbuild on Windows and the patch does. That's as far as I went with checking what the patch actually does. I'll take another look.
I think this is less about Eio specifically, and more that we care about good Windows support in Eio. With opam-repository-mingw begin discontinued for nearly two years and opam.2.2.0 around the corner, I look at this more as helping the "OCaml on Windows" effort more than just Eio. Some popular packages (e.g.
Yep, that's a reasonable request, I'll try to do that and open a new PR upstream on ocamlbuild. |
|
In amongst my pontifications on Discuss is a little bit of the context (the dates mentioned in it have gone up in flames somewhat with my health...). I think the point is that many people "cared" about the patch, they just didn't know they did 🙂 The patch was automatically available when opam was initialised using the only available distribution of it on Windows. Breaking opam-repository's normal policy on non-upstreamed patches and letting that patch go here would allow a return to that status quo. I agree that formatting the patches into a commit series is a good idea (see, for example, my doing this with the patches in opam-repository-mingw for the compiler itself) but just to add that that doesn't immediately yield a PR... there's then the decision on whether they're the patches which are wanted. Various of the patches in opam-repository-mingw for the compiler, ocamlfind and ocamlbuild patch those tools in order to work around inadequacies of the build systems of other packages - i.e. rather than carrying patches for n packages in opam-repository-mingw, workaround patches were applied further up the graph. I can see the sense of that when maintaining something which is entirely a fork, but it does mean that the patches don't necessarily want to go upstream "as is". (that's all opinion, obviously!) |
| install: [ | ||
| [make "install"] { os = "win32" } | ||
| ["mkdir" "-p" "%{lib}%/ocamlbuild"] { os = "win32" } | ||
| ["install" "-m" "0644" "META" "%{lib}%/ocamlbuild"] { os = "win32" } | ||
| ] |
There was a problem hiding this comment.
I'm not sure if opam will realise on non-Windows that this package doesn't have an install section (@rjbou?) which technically does impact non-Windows users.
Could we get away with adding an additional package for Windows (say ocamlbuild.0.14.2+fdopen) and marking this one as available : os != "win32"?
There was a problem hiding this comment.
The solver will select the +win variant anyway, but is it worth having available: os != "win32" in ocamlbuild.0.14.2/opam just for "belt and braces"?
|
Quick update, I managed to get a working version of the code which is ocaml/ocamlbuild#70 + ocaml/ocamlbuild@cb12e40 which is part of the patch in opam-repository-mingw. This seems to fix the file finding problems and the latter commit fixes the hygiene problems (currently described in ocaml/ocamlbuild#316). Hopefully this is a less invasive change that fixes the immediate issues at hand. |
|
I've rebased this patch to incorporate all of @dra27's suggestions. Can easily modify the patch to be @patricoferris', or just use the existing (and long existing) fdopen version. My preference would be to merge this PR with fdopen's patch, and then for upstream to work through Patrick's simplified version at their own pace. Am open to other suggestions that move us forward... |
A variant of this patch has been lingering externally since 2016 (ocaml/ocamlbuild#70), and Windows builds don't get very far without it any more, and it's blocking development of other upstream Windows ports (ocaml-multicore/eio#530). So barring strong objections, I'd like to pull this into opam-repository mainline. This package is only available on Windows. Source patch from https://github.com/fdopen/opam-repository-mingw/blob/opam2/packages/ocamlbuild/ocamlbuild.0.14.2/opam Reviewed by @dra27 and @patricoferris
Agreed - fdopen's patch is already extensively battle-tested... who knows, by the time there's a new release with a simpler patch, we may have Windows package testing up and running 🙂 |
|
Thanks everyone for the feedback and reviews. I'll leave this open for a few days to solicit any lingering feedback and testing, and then merge. |
| ] | ||
| } | ||
| extra-source "ocamlbuild-0.14.2.patch" { | ||
| src: "https://raw.githubusercontent.com/ocaml-opam/opam-repository-mingw/354a87b397856f2a70024c5c83fc5001074935b6/packages/ocamlbuild/ocamlbuild.0.14.2/files/ocamlbuild-0.14.2.patch" |
There was a problem hiding this comment.
should we have this file directly in the tree instead?
There was a problem hiding this comment.
exactly the opposite review above from @dra27 ;-) By holding it externally, we dont hold a fairly large patch in opam-repo, so I think external is better in this case.
There was a problem hiding this comment.
The only downside of external sources is if they disappear (this happened with a very old compiler patch, IIRC), but in general in-tree is not great for patches both because it forces all users to download them and because often the patches end up being applied to more than one package, so they get duplicated on users' machines as well.
There was a problem hiding this comment.
Or if GitHub/git changes the patch files (cf. #23837) but note that this change here isn't affected because we're referencing a patch file in a repo, rather than a commit.
There was a problem hiding this comment.
What if we move it to the opam-source-archive so that at least we control the repo where it comes from? (ah ok, I did not notice this was in @dra27 repo)
There was a problem hiding this comment.
Indeed, it's in ocaml-opam/opam-repository-mingw rather than fdopen's. We could move it - I did have in mind many months ago to put a branch for patches in opam-repository itself, but I haven't moved forward with that proposal. ocaml-opam/opam-repository-mingw will hopefully at some point be "archived", but it should never be deleted.
|
FYI, here are two CI runs failing to install camlbuild.0.14.2+win |
This patch has been lingering externally since 2016 (ocaml/ocamlbuild#70), and Windows builds don't get very far without it any more, and it's blocking development of other upstream Windows ports (ocaml-multicore/eio#530).
So barring strong objections, I'd like to pull this into opam-repository mainline. No version bump required since it only patches Windows, which is broken anyway without this fix.
Source patch from https://github.com/fdopen/opam-repository-mingw/blob/opam2/packages/ocamlbuild/ocamlbuild.0.14.2/opam
cc @dra27 @gasche @patricoferris