Skip to content

Windows - Response Files: Turn off response files for ocamlmklib#1268

Merged
rgrinberg merged 1 commit intoocaml:masterfrom
bryphe:bryphe/ocamlmklib-response-file
Sep 17, 2018
Merged

Windows - Response Files: Turn off response files for ocamlmklib#1268
rgrinberg merged 1 commit intoocaml:masterfrom
bryphe:bryphe/ocamlmklib-response-file

Conversation

@bryphe
Copy link
Copy Markdown
Contributor

@bryphe bryphe commented Sep 14, 2018 •
edited
Loading

Issue: Testing the latest master, I saw error messages building lwt@4.1.0:

Unknown option -args0
Don't know what to do with C:\Users\bryph\AppData\Roaming\npm\node_modules\esy\node_modules\esy-bash\.cygwin\tmp\responsefilea40798.data
Usage: ocamlmklib [options] <.cmo|.cma|.cmx|.cmxa|.ml|.mli|.o|.a|.obj|.lib|.dll|.dylib files>

Full error log

Fix: It turns out that ocamlmklib does not support response files at this time, so we should remove it from the whitelist of commands to consider for response files.

cc @diml - please let me know if I should add tests or tweak anything. Thank you!

@bryphe bryphe requested a review from a user September 14, 2018 00:02
@nojb
Copy link
Copy Markdown
Collaborator

nojb commented Sep 14, 2018

I will submit a PR to add this support to ocamlmklib in 4.08.

@nojb nojb mentioned this pull request Sep 14, 2018
Merged
@nojb
Copy link
Copy Markdown
Collaborator

nojb commented Sep 14, 2018

ocaml/ocaml#2045 has been merged so you may want to amend this PR to only deactivate response files for < 4.08.

@bryphe
Copy link
Copy Markdown
Contributor Author

bryphe commented Sep 14, 2018

@nojb - wonderful. Thank you for making that change!

I've updated the logic to check the version and conditionally enable response files for ocamlmklib if the version is > 4.08.0. Please let me know if there is any feedback.

@rgrinberg
Copy link
Copy Markdown
Member

rgrinberg commented Sep 15, 2018

@bryphe please add a DCO to your commits as per https://github.com/ocaml/dune/blob/master/CONTRIBUTING.md

@ghost
Copy link
Copy Markdown

ghost commented Sep 17, 2018

@bryphe thanks for the patch. We'd like to include it in the 1.2.1 release today, could you add a DCO as @rgrinberg suggested? We need it for all contributions to dune

@bryphe
Turn off response files for ocamlmklib for versions < 4.08.0
e317c27 
Signed-off-by: Bryan Phelps <bryphe@outlook.com>
@bryphe bryphe force-pushed the bryphe/ocamlmklib-response-file branch from 6e8c62c to e317c27 Compare September 17, 2018 15:13
@bryphe
Copy link
Copy Markdown
Contributor Author

bryphe commented Sep 17, 2018 •
edited
Loading

Thanks @rgrinberg and @diml for including this in 1.2.1! I've cleaned up the commits and added the DCO.

@rgrinberg rgrinberg merged commit b50b753 into ocaml:master Sep 17, 2018
@rgrinberg rgrinberg added the windows Issues that relate to Dune on Microsoft Windows label Dec 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@emillon emillon Awaiting requested review from emillon

@rgrinberg rgrinberg Awaiting requested review from rgrinberg

Assignees

No one assigned

Labels

windows Issues that relate to Dune on Microsoft Windows

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@bryphe @nojb @rgrinberg