Skip to content

Conversation

@shym
Copy link
Contributor

@shym shym commented Sep 26, 2025

This PR is a draft, in the sense that it’s probably not meant to be merged as is, as it is more of a prototype than a finished version. But it would address #151.

This PR adds an opatch command that uses the patch library and is largely extracted from opam (so kudos to the original authors there!). I think it is of more general use than just ocaml-solo5 (at least ocaml-unikraft is of interest here :-)) so it might be nice to push it into another repository, maybe patch itself if the command is good enough. I’ve cut a few corners as not everything was actually useful for our use case (in particular to delete directories that end up empty after patching).

As half of opatch is extracted from opam, it inherits its LGPL license; if we want to relicense it under ISC like patch, we would need the original authors’ agreement (apply is mostly @kit-ty-kate’s code, if I’m not mistaken; realpath has more history) or rewrite them.

@kit-ty-kate
Copy link
Member

Concerning the license, real_path is the only function taken from opam that's not my code and it is not needed since Unix.realpath is available in the OCaml versions that this project support, so once removed there should only be code with my copyright (Copyright 2024-2025 Kate Deplaix) + yours, which i'm happy to re-license to BSD-1-Clause or any similar permissive license for this project

@hannesm
Copy link
Member

hannesm commented Sep 26, 2025

there's nothing against a binary (opatch) beimg part of the patch library. I thought there already is such a binary in the repository https://github.com/hannesm/patch/blob/main/src/patch_command.ml (but i don't know whether that's sufficient or what are @kit-ty-kate nice enhancements in opam (and/or here)). :)

@shym
Copy link
Contributor Author

shym commented Oct 3, 2025

I thought there already is such a binary in the repository […]

@hannesm: IIUC, that binary is very limited (patching exactly one file, in particular), to run some tests only.

i'm happy to re-license to BSD-1-Clause or any similar permissive license for this project

@kit-ty-kate: so ISC would suit you (they seem fairly close to my non-lawyer eyes)?

(I have a series of improvements in mind for opatch, which would remove in particular the realpath function)

@kit-ty-kate
Copy link
Member

@kit-ty-kate: so ISC would suit you (they seem fairly close to my non-lawyer eyes)?

yes

@shym
Copy link
Contributor Author

shym commented Oct 3, 2025

The new version is here: hannesm/patch#35
I hope I fixed all the bugs I had introduced by the a-bit-too-quick extraction from opam’s code. It still correctly applies our OCaml/Solo5 patches, at least.

@shym shym mentioned this pull request Oct 14, 2025
@hannesm
Copy link
Member

hannesm commented Oct 22, 2025

opatch is released now, would you mind to finish this PR and add the opatch dependency? thanks

shym added 2 commits October 22, 2025 10:18
Regenerate the patch series for OCaml 5.3.0 with:
```
git format-patch --no-binary --text ...
```
so that the modifications to `configure` use a plain text diff
@shym
Copy link
Contributor Author

shym commented Oct 22, 2025

I’ve updated this PR with the released opatch but I can’t build a unikernel with it. I think that opam-monorepo is failing because it sees opatch and patch as dependencies (I thought switch dependencies were filtered, but probably later) and it’s adding its “must-be-same-source-archive” constraint. (sigh)

@hannesm
Copy link
Member

hannesm commented Oct 22, 2025

@shym that's too bad. I can see some ways forward:

@shym
Copy link
Contributor Author

shym commented Oct 22, 2025

All 3 seems good to me! Option 1 could be used only as a last resort, if both other options get stuck for long and we really want that fix in a new OCaml/Solo5, I’d say.

@hannesm
Copy link
Member

hannesm commented Oct 22, 2025

@shym great. To move forward with option 2, a review (+ testing) of that PR would be very welcome.

@hannesm
Copy link
Member

hannesm commented Nov 6, 2025

We're close, there's now a fresh release ocaml/opam-repository#28854

@kit-ty-kate
Copy link
Member

Does opatch really need to be in the same repository as patch? As far as i can see it only depends on its public interface

@hannesm
Copy link
Member

hannesm commented Nov 6, 2025

Does opatch really need to be in the same repository as patch? As far as i can see it only depends on its public interface

I honestly don't care much. If someone cares, please create a new repository and extract opatch, cut a release, and send a PR to the patch repository to remove opatch.

@shym
Copy link
Contributor Author

shym commented Nov 6, 2025

I honestly don't care much.

Neither do I. I had exposed which pros I saw for the using just one repo in the PR, and I still think those are valid reasons, though.

@shym shym marked this pull request as ready for review November 17, 2025 15:50
@shym
Copy link
Contributor Author

shym commented Nov 17, 2025

I marked this PR as ready: now that opatch 3.1 is released, I think it’s good to go.

@dinosaure
Copy link
Member

If everyone is happy with this PR, I'm ok to merge it. @hannesm, WDYT?

@hannesm
Copy link
Member

hannesm commented Nov 19, 2025

thanks for your work.

@hannesm hannesm merged commit 78a4279 into mirage:main Nov 19, 2025
2 checks passed
@shym shym deleted the opatch branch November 20, 2025 11:23
hannesm added a commit to hannesm/opam-repository that referenced this pull request Nov 25, 2025
* Support OCaml 5.4.0 (@shym mirage/ocaml-solo5#155)
* Add test of (host) ocaml-installed fmt in one example (@dinosaure mirage/ocaml-solo5#155)
* Use plain text diffs, use opatch instead of git apply (@shym @dinosaure @hannesm mirage/ocaml-solo5#154)
* Cleanup opam package dependencies (@shym mirage/ocaml-solo5#156)
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