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

Fix Windows build #15

Open
XVilka opened this issue Dec 4, 2020 · 13 comments
Open

Fix Windows build #15

XVilka opened this issue Dec 4, 2020 · 13 comments

Comments

@XVilka
Copy link

XVilka commented Dec 4, 2020

Currently it's marked as incompatible by @fdopen: fdopen/opam-repository-mingw@23748b9

It's required for enabling Windows builds for BAP: BinaryAnalysisPlatform/bap#1254

@c-cube
Copy link
Member

c-cube commented Dec 4, 2020

What breaks on windows? I don't have a windows machine on hand, so help would be appreciated.

@ivg
Copy link

ivg commented Dec 4, 2020

Here are the details, antirez/linenoise#113 it looks like that there are workarounds, i.e., versions of linenoise that we can compiler under windows (referenced in the issue). See also https://github.com/ocaml-community/ocaml-linenoise/blob/master/src/linenoise_src.c#L49

Also, I think that it would be OK, at least for starters, to just fallback dump input on windows.

@c-cube
Copy link
Member

c-cube commented Dec 4, 2020

Also, I think that it would be OK, at least for starters, to just fallback dump input on windows.

I'm swamped right now (new baby!!) but if you're able to contribute that it'd be awesome.

@c-cube
Copy link
Member

c-cube commented Jan 19, 2021

I think using redox liner for example would be better on the long term. It's more manageable than a C codebase, especially wrt unicode.

@ivg
Copy link

ivg commented Jan 19, 2021

But this will require the presence of a rust compiler/package manager and will make packing of linenoise a bit convoluted.

@c-cube
Copy link
Member

c-cube commented Jan 19, 2021

It would require cargo, yes. The rest of the packaging wouldn't change much (you can vendor the entire tree of the project).

@ivg
Copy link

ivg commented Jan 19, 2021

Yep, and requiring cargo is a much bigger hassle than requiring a C compiler (which is already required by opam itself). I don't really think that unfolding a new full toolchain is justified for linenoise which is considered as a lightweight drop in tool. Also, the name itself will no longer be correct :) I mean having a different readline-like utility with the redox backing is probably a good idea. But switching from linenoise to redox as the main backend for ocaml-linenoise doesn't sound that much good and will prevent at least myself from using it.

Besides, I am still planning to resolve this issue, just finishing stuff from the last year. My main plan is to adopt one of the portability clones of linenoise referenced in antirez/linenoise#113

It doesn't look like a lot of work, except that it was more than 20 years ago when I built something on Windows)

@c-cube
Copy link
Member

c-cube commented Jan 19, 2021

If you think you can fix the C, then power to you!! 😁

@pmetzger
Copy link
Member

pmetzger commented Apr 4, 2021

@ivg Do you want to take a crack at it?

@EmmaJaneBonestell
Copy link

It's a bit of a mess since it looks like they're all based on different versions of the upstream linenoise, but there is a working fork of linenoise with win32 support (https://github.com/msteveb/linenoise), with some missing/fixed features in a yet to be added pull request here: msteveb/linenoise#27 . It was possible for me to get ocaml-linenoise to easily compile using this fork.

@c-cube
Copy link
Member

c-cube commented Jan 4, 2022

@EmmaJaneBonestell just to make it clear, does this fork also handle the features we added, like ctrl-r? At a glance it seems like it but I'm not sure.

@EmmaJaneBonestell
Copy link

EmmaJaneBonestell commented Jan 5, 2022

@c-cube With the pull request, I believe so, yes. I haven't done any extensive testing, but it seems to work all right. It seems like it would be easier to implement/lift needed Windows code from this. The fork added lots of fixes/cleanups like 11 and 9 years ago, and I don't know if they are still relevant, should be kept, etc. (e.g. escaping in linenoiseHistoryLoad.) freehistory is named linenoiseHistoryFree and has two more lines of code for some supposed bug fixes, and so on.

For anyone who comes across this and needs a working repo (not guaranteed to match all features/be bug free):
https://github.com/EmmaJaneBonestell/ocaml-linenoise

@ivg
Copy link

ivg commented Jun 29, 2022

@EmmaJaneBonestell, would you mind making a pull request to this repository?

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

5 participants