Skip to content

Conversation

@shym
Copy link
Contributor

@shym shym commented Oct 10, 2025

This PR proposes to add a new opatch command-line tool to implement a pure OCaml alternative to GNU patch.

The proposal is to add this new command to the patch repository so that:

  • code can refactorised easily between the opatch command and the patch library if/when that makes sense (in particular, the same licence has been chosen to make sure this is possible),
  • opatch can be used (eventually) in the test suite.

It is added as a new package so that:

  • opatch can depend on a much more recent OCaml version (I’d rather add support for older versions only if there’s an actual need, and using stdcompat in that case, probably),
  • users of the patch library don’t end up with a new command-line tool they might didn’t want,
  • opatch is currently a lot less tested than the rest of the patch library; even if it is heavily based on @kit-ty-kate’s code in opam, all the changes I’ve made where not yet tested as thoroughly.

The initial goal of this new command is to have a portable patch tool for MirageOS OCaml compilers.

patches
with Invalid_argument msg -> Printf.eprintf "Fatal error: %s\n" msg

let () = main Version.v
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can write %%VERSION%% and the dune subst will replace it -- so there's no need for the custom dune rule, and generating a version.ml

Copy link
Contributor Author

@shym shym Oct 10, 2025

Choose a reason for hiding this comment

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

I got bitten by this because this works only in dev mode. dune subst doesn’t run on release tarballs (which is reasonable as it pulls info from git describe). So the packages that use those placeholders generate explicit release tarballs where they are substituted. I find this trick simpler and lighter.

Copy link
Owner

Choose a reason for hiding this comment

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

Hmm, ok. My experience is when use dune-release these watermarks get replaced. But if you prefer the version.ml generated by dune rule, let's go with that.

Copy link
Collaborator

@kit-ty-kate kit-ty-kate Oct 10, 2025

Choose a reason for hiding this comment

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

I'd rather not use dune subst

Copy link
Contributor Author

@shym shym Oct 10, 2025

Choose a reason for hiding this comment

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

In the aftermath of the xz-attack, I got convinced (by a talk? a post somewhere? I forget :-/) that the changes between the tagged version of the source and the actual release tarball are harmful. So much so that I’m advocating using automatically github-generated tarballs instead of user-created tarballs, to remove one link in the trust chain.

This patch reuses a lot of code written by Kate Deplaix for opam
@shym
Copy link
Contributor Author

shym commented Oct 10, 2025

I’ve tested this command mostly with the patches for the compiler in OCaml/Solo5.
@dinosaure: I think I remember you had larger patch series, it would be interesting to see if they are applied correctly too.

@dinosaure
Copy link

Can not really test but don't hesitate to go forward without me on this topic.

@hannesm
Copy link
Owner

hannesm commented Oct 15, 2025

Thanks. So how to move forward? Since we have some tests, could we use opatch there? From a maintenance point of view, I'd be in favour to remove src/patch_command.ml.

About dependencies and complexity, I would be happy to merge this into the patch library --- even if you plan to depend on cmdliner. Are there any downsides of having it in the same opam package?

@kit-ty-kate
Copy link
Collaborator

Are there any downsides of having it in the same opam package?

avoiding the extra dependency would be great. I also personally don't think we should mix binaries and libraries in the same package

@hannesm
Copy link
Owner

hannesm commented Oct 15, 2025

@kit-ty-kate which extra dependency are you talking about?

I also don't quite understand why mixing library and binary would be a bad idea - is there some reasoning behind that?

@shym
Copy link
Contributor Author

shym commented Oct 15, 2025

From a maintenance point of view, I'd be in favour to remove src/patch_command.ml.

According to git grep, patch_command is not actually used in tests. I suppose it has been useful in debugging the counter-examples generated by the crowbar-based test.

About dependencies and complexity, I would be happy to merge this into the patch library --- even if you plan to depend on cmdliner. Are there any downsides of having it in the same opam package?

I don’t plan to add another dependency, but opatch is written targetting Stdlib 4.14, not 4.08. I’m not convinced about the point of supporting older versions for the binary. As the library is used by opam, it must allow for older versions, I guess.

@kit-ty-kate
Copy link
Collaborator

@kit-ty-kate which extra dependency are you talking about?

ah nevermind i thought this was depending on cmdliner

is there some reasoning behind that?

mostly the rule of least surprise. If someone calls opam install patch i would expect to get the patch library. If i call opam install opatch i would expect the binary. It also limits what the binary can depend on (cmdliner as you said, which i would be against if it was added as a dependency of the patch package)

@hannesm
Copy link
Owner

hannesm commented Oct 15, 2025

Ok, thank you. I'll merge and release (opatch only).

@hannesm hannesm merged commit e95238b into hannesm:main Oct 15, 2025
1 check was pending
hannesm added a commit to hannesm/opam-repository that referenced this pull request Oct 15, 2025
hannesm added a commit to hannesm/opam-repository that referenced this pull request Oct 15, 2025
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