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

Add support for -args and -args0 to ocamlmklib #2045

Merged
merged 2 commits into from Sep 14, 2018

Conversation

Projects
None yet
3 participants
@nojb
Copy link
Contributor

commented Sep 14, 2018

This PR adds options -args and -args0 to ocamlmklib, similar to the other tools.

See also ocaml/dune#1268 and ocaml/dune#1256.

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

This is not directly related to your change but: why is ocamlmklib parsing arguments by hand instead of using Arg to do the job?

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2018

Good question. I am not sure, but I suspect it is due to the slightly clever handling of some arguments (e.g. -Wl,-rpath) which may not be easily doable with Arg.

@@ -166,6 +177,10 @@ let usage = "\
Usage: ocamlmklib [options] <.cmo|.cma|.cmx|.cmxa|.ml|.mli|.o|.a|.obj|.lib|\
.dll|.dylib files>\
\nOptions are:\
\n -args <file> Read additional newline-terminated comman line arguments\

This comment has been minimized.

Copy link
@dbuenzli

dbuenzli Sep 14, 2018

Contributor

Typo (comman).

This comment has been minimized.

Copy link
@nojb

nojb Sep 14, 2018

Author Contributor

Thanks, fixed.

@gasche

gasche approved these changes Sep 14, 2018

Copy link
Member

left a comment

I have reviewed the code and believe it is correct, but made some code-shuffling suggestions.

Not to be merged before @nojb says it is ready.

for i = Array.length arr - 1 downto first do
Stack.push arr.(i) stk
done
;;

This comment has been minimized.

Copy link
@gasche

gasche Sep 14, 2018

Member

I would move this within parse_arguments to be next to next_arg, and use a label for the first argument for readability of the callsites.

This comment has been minimized.

Copy link
@nojb

nojb Sep 14, 2018

Author Contributor

Yes, done.

Show resolved Hide resolved tools/ocamlmklib.ml Outdated

@nojb nojb force-pushed the nojb:ocamlmklib_args branch from 15f870c to fe8013d Sep 14, 2018

@gasche

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

(I'm happy with the refactoring; @nojb is it ready on your side?)

@nojb

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2018

Yes, ready!

@gasche gasche merged commit 1056f6b into ocaml:trunk Sep 14, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gasche

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

Merged, thanks!

@dra27 there is something suspicious in the AppVeyor logs:

-=-=- End of install msvc64 -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
-=-=- check_all_arches -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
make: Entering directory '/cygdrive/c/projects/🐫реализация-msvc64'
fatal: Not a git repository: /cygdrive/c/projects/🐫реализация-msvc64/C:/projects/ocaml/.git/worktrees/🐫реализация-msvc64
fatal: Not a git repository: /cygdrive/c/projects/🐫реализация-msvc64/C:/projects/ocaml/.git/worktrees/🐫реализация-msvc64
========= CHECKING asmcomp/amd64 ==============
fatal: Not a git repository: /cygdrive/c/projects/🐫реализация-msvc64/C:/projects/ocaml/.git/worktrees/🐫реализация-msvc64
fatal: Not a git repository: /cygdrive/c/projects/🐫реализация-msvc64/C:/projects/ocaml/.git/worktrees/🐫реализация-msvc64
[...]
========= CHECKING asmcomp/s390x ==============
fatal: Not a git repository: /cygdrive/c/projects/🐫реализация-msvc64/C:/projects/ocaml/.git/worktrees/🐫реализация-msvc64
make: Leaving directory '/cygdrive/c/projects/🐫реализация-msvc64'
-=-=- End of check_all_arches -=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
Build success
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.