Skip to content

Make ocamlmklib fail on unknown parameter#13638

Merged
gasche merged 1 commit intoocaml:trunkfrom
dra27:ocamlmklib-unknown
Apr 29, 2025
Merged

Make ocamlmklib fail on unknown parameter#13638
gasche merged 1 commit intoocaml:trunkfrom
dra27:ocamlmklib-unknown

Conversation

@dra27
Copy link
Copy Markdown
Member

@dra27 dra27 commented Nov 26, 2024

In the spirit of #12613. The original ocamlmklib shell script used to ignore command line options it didn't recognise (I'm going to guess for build system simplicity). The present OCaml rewrite of it was introduced in OCaml 3.04 and temporarily did return an error status, but this got "corrected" in 31cbd26.

I can see why this might have been acceptable 20+ years ago, but I don't think it's good or consistent (and I just got bitten by it while trying to write some sanity-checking tests...). The rest of the distribution returns a non-zero status when passed on invalid argument - so should ocamlmklib!

(cc @garrigue, but only because the original commit is yours!)

@damiendoligez damiendoligez self-assigned this Nov 27, 2024
@damiendoligez damiendoligez self-requested a review November 27, 2024 14:40
@shindere
Copy link
Copy Markdown
Contributor

Thanks a lot. Approving but not merging, so that @dra27 can
complete the Changes entry.

What about making the commit message a bit more explicit, at least
saying that it's ocamlmklib the commit is about?

Independently of this PR, I noticed that ocamlmklib does not
seem to have a man page. Is it on purpose? If not, is it okay at
the moment to just open an issue to track this?

@dra27 dra27 force-pushed the ocamlmklib-unknown branch from 09b00c6 to 87d1f6b Compare January 8, 2025 13:51
@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Jan 8, 2025

Independently of this PR, I noticed that ocamlmklib does not
seem to have a man page. Is it on purpose? If not, is it okay at
the moment to just open an issue to track this?

I don't expect so (Debian adds one) - although I'm not sure this is something we should do prior to overhauling the way they're generated (it's already necessary to keep two places in sync, given the documentation in the "Interfacing with C" chapter and also the --help output).

@dra27
Copy link
Copy Markdown
Member Author

dra27 commented Apr 14, 2025

Gentle ping for @damiendoligez

@dra27 dra27 force-pushed the ocamlmklib-unknown branch from 87d1f6b to 99bf62e Compare April 24, 2025 22:14
@gasche gasche merged commit c9ee4f4 into ocaml:trunk Apr 29, 2025
22 checks passed
@gasche
Copy link
Copy Markdown
Member

gasche commented Apr 29, 2025

I didn't see a good reason to wait indefinitely on this one, so I went ahead and merged.

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.

5 participants