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

Windows support #684

Merged
merged 18 commits into from Aug 21, 2017

Conversation

Projects
None yet
4 participants
@dra27
Copy link
Contributor

commented Jul 27, 2017

This is a work-in-progress towards native Windows support for Merlin 3.x - I've only so far tested OCaml 4.04.2 on mingw. There are various tasks remaining:

  • ml_merlin_fs_exact_case - this is presently for MacOS, but I think the same thing may be required for Windows
  • The code in src/frontend/ocamlmerlin_server.ml for Server.loop checks for an upgrade to ocamlmerlin-server on each loop. This won't work on Windows, since the executable can't be overwritten while it's running. Probably need more documentation or possibly a build system update to try ocamlmerlin server stop-server in make install
  • MS CRT's stat doesn't return unique inode numbers - I need to use the same code as in OCaml's stat to get a meaningful unique pipe name
  • I haven't tested whether this works as an unprivileged user yet (may require a workaround for named pipes - I can't remember)
  • The logic behind GetFullPathName/GetLongPathName use is a bit dodgy.
  • Even with CreateProcess, the server needs dummy handles to ensure it doesn't inherit the client's file handles (which therefore won't be closed). This is more of a nuisance when debugging and testing than using in an editor. Note that on Windows you cannot pass console handles to another process, so you must always redirect stdout and stdin for the server protocol to work. (edit: it turns out it doesn't, being detached appears to be enough)
  • AppVeyor

A couple of questions which may lead to further changes:

  • Why is ml_merlin_server_setup implemented in C? (curiosity...)
  • I think there's no buffer overrun check in recv_buffer?

dra27 added some commits Jul 26, 2017

Trivial Windows fix to uninstall target
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Also uninstall ocamlmerlin-server
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Fix detection of src/ocaml/typer
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Some of us actually use Vim...
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Use ln -s on Windows when it works
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Rename abnormal_termination
The symbol conflicts with part of the Microsoft C runtime.

Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Cygwin \r fixes to OCamlMakefile
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Use correct $(CC) on Windows
Signed-off-by: David Allsopp <david.allsopp@metastack.com>

@dra27 dra27 changed the title Windows supportcd .. Windows support Jul 27, 2017

Windows support
Signed-off-by: David Allsopp <david.allsopp@metastack.com>

@dra27 dra27 force-pushed the dra27:windows-support branch from 80029ed to aae5c4e Jul 27, 2017

@let-def

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

Thanks, that's an awesome contribution.

Why is ml_merlin_server_setup implemented in C? (curiosity...)

I recall two reasons, but they might be wrong and are certainly no longer applicable:

  • the setup has been refactored, a C function made sense at the time but became less relevant later
  • since this function was dealing with system specific things, it was easier to let C handle (and its preprocessor) handle platform specific code and have a single function in OCaml.

I think there's no buffer overrun check in recv_buffer?

Indeed!

@dra27 dra27 force-pushed the dra27:windows-support branch 4 times, most recently from 6cf70e4 to 4a90774 Jul 27, 2017

dra27 added some commits Jul 27, 2017

AppVeyor Test Scripts
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
MSVC Posix-compatibility definitions
Add definitions for STD*_FILENO, ssize_t, snprintf and PATH_MAX.

Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Correct C compiler invocation for MSVC
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Update .gitignore for MSVC
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Fix disabling Emacs compilation
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Tidy handling of .exe in build system
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Add ocamlmerlin-test to .gitignore
Signed-off-by: David Allsopp <david.allsopp@metastack.com>
Fix path handling in test suite on Windows
Signed-off-by: David Allsopp <david.allsopp@metastack.com>

@dra27 dra27 force-pushed the dra27:windows-support branch from 4a90774 to 19eadbb Jul 30, 2017

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2017

Right, three new things:

  1. Builds reliably on MSVC (including on old MSVC - i.e. Windows 7 SDK)
  2. The testsuite is now equivalently failing on Windows as on other platforms
  3. AppVeyor scripts added
@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2017

The AppVeyor stuff warrants a bit of further explanation:

  • It tests 4.02.3, 4.03.0, 4.04.2 and 4.05.0 on all 4 Windows ports (i.e. 16 builds)
  • You obviously can't ever build that many compilers each time. AppVeyor caps execution time to an hour, so it can't even be done once in one go, so the scripts are set so that they incrementally build and cache compilers on each successive run. As long as you don't update a dependency or request a new version, then it converges on a full test. You can see this in action on my own fork:
    • 1.0.97 - the cache had OCaml 4.02.2 and 4.04.1 but the commit requested an upgrade to 4.02.3 and 4.04.2. It therefore built 4.02.3 but skipped 4.04.2 (note Messages tab as well)
    • 1.0.98. AppVeyor was told to build the same commit again - this time it builds 4.04.2 and so tests them all (40 minute build)
    • 1.0.99. Again, AppVeyor was told to restart but this time the caches are all used and the full test on all 16 compilers takes 17 minutes, rather than 40.

Obviously, AppVeyor needs to be enabled for ocaml/merlin for this to be of any use!

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2017

@let-def - I'm not sure when I'll have time to work on the other 4 issues, but none of them are blockers. An option could be to review all this code as one now, and file 4 issues to track these other items? I also haven't done any testing of emacs

@let-def let-def force-pushed the ocaml:master branch 2 times, most recently from 177d839 to fd21902 Aug 2, 2017

@let-def let-def referenced this pull request Aug 2, 2017

Closed

Compiling on Windows #692

Correct definitions of STD*_FILENO for Windows
The definitions using _fileno return -1 or -2 for a detached process
(apparent difference in behaviour between Windows 7 and Windows 10)

Signed-off-by: David Allsopp <david.allsopp@metastack.com>

@dra27 dra27 force-pushed the dra27:windows-support branch from 19eadbb to 22f4684 Aug 17, 2017

@let-def let-def merged commit 32f59d0 into ocaml:master Aug 21, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bluddy

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2017

If merlin works on windows again, I recommend releasing a new version on opam asap.

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2017

Possibly, although opam pin add merlin --dev-repo should work just fine too.

@kkirstein

This comment has been minimized.

Copy link

commented Aug 25, 2017

Just tried it on my Windows box (Win10-64bit, rel 1703, cygwin environment, mingw toolchain). After opam pin add merlin --dev-repo, merlin compiles out of the box. Vim integration works, but atom & vscode not (atom thows exception, vscode silently does nothing).
IMO, releasing would be o.k., but the protocol change of merlin 3.x still causes some problems. I will have a look at the respective vscode project...

@dra27

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2017

Thanks @kkirstein! It'd certainly be good to sync up an actual release with support for other editors (I haven't tested emacs on Windows, either).

In principle, atom and vscode should be working in legacy mode, though it would obviously be good to switch them over to the new protocol - let me know if I can help!

@bluddy

This comment has been minimized.

Copy link
Contributor

commented Aug 28, 2017

since this function was dealing with system specific things, it was easier to let C handle (and its preprocessor) handle platform specific code and have a single function in OCaml.

@let-def It would be nice to put a to-do item to move the server to OCaml, and sort out whatever needs doing (perhaps in stdlib or some other library) so that others can benefit from / rely on this work.

@kkirstein

This comment has been minimized.

Copy link

commented Aug 29, 2017

@dra27 It looks like that legacy mode does not proper work with the current atom & vscode extensions. I wanted to approach the respective projects with this issue, but unfortunately I am quite busy these days...

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.