Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ocamlbuild for windows OCaml 5.2 #25939

Merged
merged 1 commit into from
Jun 3, 2024
Merged

Conversation

hhugo
Copy link
Contributor

@hhugo hhugo commented May 26, 2024

This PR provides a windows variant of ocamlbuild.0.14.3.
It re-uses the patch used to fix ocamlbuild.0.14.2 for windows and make ocamlbuild.0.14.3 available on windows with OCaml 5.02.

@hhugo
Copy link
Contributor Author

hhugo commented May 26, 2024

cc @kit-ty-kate @gasche

@hhugo
Copy link
Contributor Author

hhugo commented May 27, 2024

I've tested this in ocsigen/js_of_ocaml#1615

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather avoid having a separate package for this.
I think we should either have the patch applied conditionally on Windows in the 0.14.3 package or wait a couple weeks to have an upstream version (this is next on my todo-list after the release of opam 2.2.0~rc1)

@hhugo
Copy link
Contributor Author

hhugo commented May 27, 2024

I'd rather avoid having a separate package for this. I think we should either have the patch applied conditionally on Windows in the 0.14.3 package or wait a couple weeks to have an upstream version (this is next on my todo-list after the release of opam 2.2.0~rc1)

I don't think we should wait before making ocamlbuild available on windows 5.2. There are enough issues on the windows side already.

We could make it part of the main opam file. I don't know the rational for splitting the files initially. I just kept the setup in place to unlock the situation as fast as possible.

@hhugo
Copy link
Contributor Author

hhugo commented May 28, 2024

I think it would be enough to merge this into https://github.com/ocaml-opam/opam-repository-mingw

Maybe we should move ocamlbuild.0.14.2+win as well. What do you think ?

@kit-ty-kate
Copy link
Member

Maybe we should move ocamlbuild.0.14.2+win as well. What do you think ?

Packages can't be removed until #23789 is implemented/formally accepted.

But overall I think it would be fine as part of the main opam file. I really don't understand why it was split in the first place.
It would just be a case of doing:

patches: [
  "ocamlbuild-0.14.2.patch" {os = "win32"}
]

@hhugo
Copy link
Contributor Author

hhugo commented May 28, 2024

Maybe we should move ocamlbuild.0.14.2+win as well. What do you think ?

Packages can't be removed until #23789 is implemented/formally accepted.

But overall I think it would be fine as part of the main opam file. I really don't understand why it was split in the first place. It would just be a case of doing:

patches: [
  "ocamlbuild-0.14.2.patch" {os = "win32"}
]

I guess the +win variant makes it clear that it's a modified version. There are a fair amount of changes in this patch. Changes that have never been reviewed or proposed upstream.

@hhugo
Copy link
Contributor Author

hhugo commented May 28, 2024

That said, if you prefer to merge the patch in the main opam, it's fine.

@mseri
Copy link
Member

mseri commented May 28, 2024

We have anouther ocamlbuild +win version already published, right? As much as I don't like this, I think we could go on with this and look for a better solution for the future?

Copy link
Member

@kit-ty-kate kit-ty-kate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fair enough

@hhugo
Copy link
Contributor Author

hhugo commented Jun 1, 2024

Can we unlock the situation on windows and merge this ?

@mseri mseri merged commit 8330238 into ocaml:master Jun 3, 2024
1 of 2 checks passed
@mseri
Copy link
Member

mseri commented Jun 3, 2024

Done. Is there any chance this patch could be reviewed and merged into upstream ocamlbuild any time soon? Windows support shouldn't be an ad-hoc opam-repo thing imho

@gasche
Copy link
Member

gasche commented Jun 3, 2024

The problem is that this patch is a hodge-podge of stuff that was only ever tested on Windows, so it feels very unsafe (to me) to merge as-is for everyone. Just splitting this into proper patches and understanding the intent of each change is a substantial amount of work. In principle I would be delighted to see this work being done, but in practice few people have the energy to do it. @hhugo has done some of it in the last few weeks, and we try to make his contributions as pleasant as possible to keep him motivated to do more :-)

@hhugo hhugo deleted the ocamlbuild-ocaml52-win branch June 3, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants