Add function channel_of_descr_socket (windows has different handles for sockets and files)

garrigue / lablgtk

LablGTK 2 and 3: an interface to the GIMP Tool Kit

https://garrigue.github.io/lablgtk

Other

89 stars 40 forks source link

Add function channel_of_descr_socket (windows has different handles for sockets and files) #176

Closed SkySkimmer closed 2 months ago

SkySkimmer commented 5 months ago

This PR upstreams a patch which was carried in windows opam repos, eg https://github.com/coq/platform/blob/main/opam/opam-repository/packages/lablgtk3/lablgtk3.3.1.3/files/0001-Add-function-channel_of_descr_socket-windows-has-dif.patch

SkySkimmer commented 5 months ago

(I don't know if this patch is expected to work outside windows or if it needs some modification)

MSoegtropIMC commented 5 months ago

I would say the main question is if we want separate functions also in Unix, so that one could more easily write portable code. If so, then on Unix the two channel_of_descr functions would be the same, on Windows not. On all OSes one should use the correct function depending on if one is working with a file or a socket. The downside of this portability would be that it would require distinctions which would be quite artificial in Linux in some playes - maybe even annoying.

garrigue commented 4 months ago

I've changed it quite a lot. Basically, since OCaml uses a common tagged data structure for sockets and handles, there is no need to have separate functions. Unfortunately, I have no environment to test that on windows. Can you check that it works ? Also, I'm not opposed to add the other ocaml function name for backward compatibility, if it matters.

SkySkimmer commented 4 months ago

I don't have a windows to test either

MSoegtropIMC commented 4 months ago

@garrigue : I can test it. Are we talking about the PR branch or can I find the code somewhere else?

garrigue commented 4 months ago

I pushed the code to the pr branch.

MSoegtropIMC commented 4 months ago

OK, give me a few days. I put it on my list for Friday.

MSoegtropIMC commented 4 months ago

Sorry, I had to delay it to Monday, but it is still on my list ...

MSoegtropIMC commented 4 months ago

Not sure what is going on, but I can't get the patch applied by opam. If I run opam reinstall -b lablgtk3 I don't see the patch applied in the opam build folder (the patch file as such is there).

I downloaded https://patch-diff.githubusercontent.com/raw/garrigue/lablgtk/pull/176.patch, added it to the opam file in the usual way (there was already one in my local patch repo - I just had to change the name). All looks good, no errors, opam lint says all is good, but the patch is not done.

If I modify the name of the patch file, opam complains that the patch file is not there (as it should). Also my original patch works. But if I put in the patch of this PR nothing happens. I have not the slightest idea what is going on here.

It might take a bit ...

MSoegtropIMC commented 4 months ago

I think I found it - the patch produced by github (see link above) contains several separate patches for separate commits (4 in total) - apparently opam / patch does not support this.

MSoegtropIMC commented 4 months ago

I split the patch file into 4 and added all 4 to opam - this also doesn't work (no errors, no patch applied).

Can someone please squash this PR - maybe this fixes it.

SkySkimmer commented 4 months ago

I don't think squashing would change the patch.

SkySkimmer commented 4 months ago

Also why test with a patch instead of pointing the url at my branch?

MSoegtropIMC commented 4 months ago

Also why test with a patch instead of pointing the url at my branch?

It is Coq Platform policy to only use official releases + patches which are kept directly in the Coq Platform repo.

But this can be handled - I will just clone the repo (I am lazy and didn't do sp as yet) and I am sure I find a way to create the patch - worst case I put both versions in separate folders ...

MSoegtropIMC commented 4 months ago

I squashed it in a local branch and created a patch from that - this did work.

One obviously needs to adjust CoqIDE then - I guess we do this from Coq 8.20 on. We need to take care to link it to the proper lablgtk versions in opam then.

I did a quick test of the resulting CoqIDE and it works fine.

=> good to go.

SkySkimmer commented 4 months ago

One obviously needs to adjust CoqIDE then

That's what

Also, I'm not opposed to add the other ocaml function name for backward compatibility, if it matters.

is about no?

MSoegtropIMC commented 4 months ago

One obviously needs to adjust CoqIDE then

That's what

Also, I'm not opposed to add the other ocaml function name for backward compatibility, if it matters.

is about no?

Yes. I am not sure we want this, though. It is for sure cleaner to get rid of it and assuming everyone is using opam it should not be a problem to manage the version restrictions. But I have no strong feelings about this - adding the old function name as an alias would also be an option.

MSoegtropIMC commented 4 months ago

Thinking about it: one would have to add an upper bound for the lablgtk version for all older versions of CoqIDE. I still would prefer this solution, but again no strong feelings.

SkySkimmer commented 2 months ago

So is this going to be merged?

garrigue commented 2 months ago

Sorry the long delay, I got to do other things and forgot. But I still need to do a release, probably next week.