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 #19

Closed
SWvonDeylen-ONT opened this issue Aug 31, 2017 · 5 comments
Closed

Windows support #19

SWvonDeylen-ONT opened this issue Aug 31, 2017 · 5 comments

Comments

@SWvonDeylen-ONT
Copy link
Contributor

Heng,

thank you so much for this enormously fast and good-quality aligner. At Oxford Nanopore Technologies, we are working on adding minimap2 as a shared library. As we are shipping our software for Linux, OS-X and Windows, we need to build on either of these platforms.

Our current approach is to build the standard minimap2 executable and a shared library which we use internally via a small cmake wrapper, see https://github.com/nanoporetech/ont_minimap2.

In order to compile with Windows, we needed to make small adjustments to the code, which might be useful for other users as well. We have made those changes in https://github.com/nanoporetech/minimap2/tree/msvc14, which we would be happy to contribute to your repository, possibly after aligning with your ideas.

Best regards,
Stefan von Deylen

@lh3
Copy link
Owner

lh3 commented Aug 31, 2017

I have briefly reviewed your commit. These look like minor changes (I would probably merge gettimeofday.c into misc.c, but I can do that myself). Could you send me a PR against minimap2 master? I am on travel and have no access to Windows machines, but I will have a look and consider to merge your PR. Thanks.

@lh3
Copy link
Owner

lh3 commented Aug 31, 2017

By the way, there are probably two memory leaks in minimap2/example.c, which I have not fixed yet. I will fix them in the next several days (as I am on travel right now). Your ont_minimap2/ont_minimap2.c may need to be updated accordingly.

@lh3
Copy link
Owner

lh3 commented Sep 3, 2017

I have merged #20 and fixed memory leaks in minimap2/example.c. The latest branch differs from your original PR in that master

  1. doesn't use alloca
  2. doesn't use variable-length arrays
  3. uses getopt_long from musl, which doesn't support multi-byte options but is much simpler. getopt_long is a GNU extension. I need to find a replacement on other OS anyway.

MSVC 14 can compile the master, though I haven't tried the linking step. Let me know if these work with your build. Thanks.

@SWvonDeylen-ONT
Copy link
Contributor Author

Thank you very much. We are now building our library for all platforms directly from minimap2 master. We are using the Intel Compiler for Windows, and it might be that MSVC would not find _InterlockedExchangeAdd() at link time.

@lh3
Copy link
Owner

lh3 commented Sep 7, 2017

MSVC seems to have a version of InterlockedExchangeAdd, but anyway, I am not familiar with the Windows build system and won't deliver a Windows build. As long as you are happy with ICC, I will keep minimap2 as it is for now. I will also be careful not to break existing APIs in future or to add features unsupported on Windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants