Add missing ocaml_terminal_get_sigwinch for unsupported platform

[art-w]

Long time no see, I hope you are doing great!

This is a quick fix for compiling for mirage... but let me know if you had something else in mind :)

[dinosaure]

From a certain perspective, it is preferable to delegate the injection of such a function to the user. It has been mentioned that:

1. progress is really not MirageOS friendly
2. terminal could be inspired by notty (which is MirageOS friendly) as far as abstractions are concerned.

Can you elaborate a bit more on your goal in relation to MirageOS?

PS: CraigFe gave me a hand on progress maintenance, happy to see for another beer 🍺

[art-w]

Yeah I was wondering if the absence was intentional, but then the function ocaml_terminal_get_terminal_dimensions already had a dummy placeholder for unsupported platforms. Would you prefer if we remove both functions, or a refactoring of terminal to avoid those hacks?

Regarding my use-case, I was trying to compile index and irmin on mirage and they both depend on progress to report progress on long running consistency checks... so this PR was only attempting to fix the linking error in a way that seemed consistent with the existing progress code :)

PS: Oh cool! Yes let's go for beers soon!

[dinosaure]

Yeah I was wondering if the absence was intentional, but then the function ocaml_terminal_get_terminal_dimensions already had a dummy placeholder for unsupported platforms. Would you prefer if we remove both functions, or a refactoring of terminal to avoid those hacks?

I don't really have an opinion on this specific subject. In my opinion, the problem is broader if we take your objective into account. In other words, if it's really a question of making a unikernel (Solo5) which includes progress, the work required should be greater than that unfortunately.

Regarding my use-case, I was trying to compile index and irmin on mirage and they both depend on progress to report progress on long running consistency checks... so this PR was only attempting to fix the linking error in a way that seemed consistent with the existing progress code :)

Currently, you should have more problems than this function since progress is explicitly Unix-dependent: in other words, only the unix target should work as far as MirageOS is concerned (you should try hvt and see all the errors).

[art-w]

Ha yes but I've switched index and irmin to use progress.engine (instead of progress, as it doesn't depend on unix) and so it seems to run fine on hvt with this short patch :)

progress.engine is already parametrized by a Platform argument, so the C stubs from terminal are actually never called on mirage: https://github.com/craigfe/progress/blob/main/src/progress/engine/platform.ml#L21 so I could move these C stubs out of terminal and put them directly in the unixy progress (this would be a breaking change for terminal interface, but it looks like only progress depends on it anyway :) )

[dinosaure]

It's an interesting idea indeed. I need to spend some time thinking through all the implications, but a proposal along these lines would be interesting to continue the discussion.

[art-w]

Added a commit to move the C stubs out of terminal, such that they are only required when using progress :) (but one can use the progress.engine functor on mirage, since it's unix and stub free!)

[dinosaure]

It seems good for me, terminal becomes more and more close to what notty provides but that's fine. Thanks! I will try to see and merge others PRs before a release but you are able to use the upstream version now 👍.