Skip to content

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

Merged
gasche merged 2 commits intoocaml:trunkfrom
nojb:ocamlmklib_args
Sep 14, 2018
Merged

Add support for -args and -args0 to ocamlmklib#2045
gasche merged 2 commits intoocaml:trunkfrom
nojb:ocamlmklib_args

Conversation

@nojb
Copy link
Copy Markdown
Contributor

@nojb nojb 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
Copy link
Copy Markdown
Member

gasche 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
Copy link
Copy Markdown
Contributor Author

nojb 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.

dbuenzli
dbuenzli reviewed Sep 14, 2018
View reviewed changes
tools/ocamlmklib.ml Outdated
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\
Copy link
Copy Markdown
Contributor

@dbuenzli dbuenzli Sep 14, 2018

Choose a reason for hiding this comment

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

Typo (comman).

Copy link
Copy Markdown
Contributor Author

@nojb nojb Sep 14, 2018

Choose a reason for hiding this comment

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

Thanks, fixed.

gasche
gasche approved these changes Sep 14, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@gasche gasche left a comment

Choose a reason for hiding this comment

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

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.

tools/ocamlmklib.ml Outdated
for i = Array.length arr - 1 downto first do
Stack.push arr.(i) stk
done
;;
Copy link
Copy Markdown
Member

@gasche gasche Sep 14, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

@nojb nojb Sep 14, 2018

Choose a reason for hiding this comment

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

Yes, done.

@gasche
Copy link
Copy Markdown
Member

gasche commented Sep 14, 2018

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

@nojb
Copy link
Copy Markdown
Contributor Author

nojb commented Sep 14, 2018

Yes, ready!

@gasche gasche merged commit 1056f6b into ocaml:trunk Sep 14, 2018
@gasche
Copy link
Copy Markdown
Member

gasche 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

@nojb nojb mentioned this pull request Sep 14, 2018
Merged
@vicuna vicuna mentioned this pull request Mar 14, 2019
Closed
@nojb nojb mentioned this pull request Aug 2, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gasche gasche gasche approved these changes

+1 more reviewer

@dbuenzli dbuenzli dbuenzli left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@nojb @gasche @dbuenzli