OpenBLAS patch for Cygwin

OpenBLAS patch for Cygwin

[Erik Bray]

Hello,

Users/maintainers of OpenBLAS on Cygwin may be interested in this
patch to improve support for fork():
https://github.com/xianyi/OpenBLAS/pull/1450

Assuming this looks good (feedback welcome) it might be nice to have
this patch included in the next release of the official OpenBLAS
package for Cygwin since its incompatibility with fork() has caused
problems in the past [1].


Thanks,
E

[1] https://trac.sagemath.org/ticket/22822

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: OpenBLAS patch for Cygwin

[Marco Atzeri]

On 06/02/2018 13:10, Erik Bray wrote:
> Hello,
> 
> Users/maintainers of OpenBLAS on Cygwin may be interested in this
> patch to improve support for fork():
> https://github.com/xianyi/OpenBLAS/pull/1450
> 
> Assuming this looks good (feedback welcome) it might be nice to have
> this patch included in the next release of the official OpenBLAS
> package for Cygwin since its incompatibility with fork() has caused
> problems in the past [1].
> 
> 
> Thanks,
> E
> 
> [1] https://trac.sagemath.org/ticket/22822
> 


Noted.
Do you have a test case to show the failure with current build ?

Any reason to use OS_CYGWIN_NT and not __CYGWIN__ in the patch ?

Regards
Marco

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: OpenBLAS patch for Cygwin

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1151 bytes --]

On Feb  6 15:15, Marco Atzeri wrote:
> On 06/02/2018 13:10, Erik Bray wrote:
> > Hello,
> > 
> > Users/maintainers of OpenBLAS on Cygwin may be interested in this
> > patch to improve support for fork():
> > https://github.com/xianyi/OpenBLAS/pull/1450
> > 
> > Assuming this looks good (feedback welcome) it might be nice to have
> > this patch included in the next release of the official OpenBLAS
> > package for Cygwin since its incompatibility with fork() has caused
> > problems in the past [1].
> > 
> > 
> > Thanks,
> > E
> > 
> > [1] https://trac.sagemath.org/ticket/22822
> > 
> 
> 
> Noted.
> Do you have a test case to show the failure with current build ?
> 
> Any reason to use OS_CYGWIN_NT and not __CYGWIN__ in the patch ?

Also, it should really use pthread functions and drop the notion that
Cygwin is a Windows target.  Assuming you're running fork from another
thread than the main thread, does it still work with native Windows
threads?


Corinna

-- 
Corinna Vinschen                  Please, send mails regarding Cygwin to
Cygwin Maintainer                 cygwin AT cygwin DOT com
Red Hat

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: OpenBLAS patch for Cygwin

[Achim Gratz]

Erik Bray writes:
> Assuming this looks good (feedback welcome) it might be nice to have
> this patch included in the next release of the official OpenBLAS
> package for Cygwin since its incompatibility with fork() has caused
> problems in the past [1].

It would be vastly preferrable if OPenBLAS ditched the (unfortunately
quite common) notion that "Cygwin is some sort of Windows" for "Cygwin
is some sort of Linux".  I've patched out quite some bit of conditionals
like that in some other packages and it was almost always for the
better.  The worst ones are those that go into the Windows conditional
branch and then on to "oh wait, but for Cygwin we need something else".


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldUserWavetables

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: OpenBLAS patch for Cygwin

[Erik Bray]

Hi, Sorry for the non-response. Been traveling for the last week and
not really checking this e-mail much.

On Tue, Feb 6, 2018 at 3:15 PM, Marco Atzeri wrote:
> On 06/02/2018 13:10, Erik Bray wrote:
>>
>> Hello,
>>
>> Users/maintainers of OpenBLAS on Cygwin may be interested in this
>> patch to improve support for fork():
>> https://github.com/xianyi/OpenBLAS/pull/1450
>>
>> Assuming this looks good (feedback welcome) it might be nice to have
>> this patch included in the next release of the official OpenBLAS
>> package for Cygwin since its incompatibility with fork() has caused
>> problems in the past [1].
>>
>>
>> Thanks,
>> E
>>
>> [1] https://trac.sagemath.org/ticket/22822
>>
>
>
> Noted.

And I see you just made a new package release incorporating my patch--thanks!

> Do you have a test case to show the failure with current build ?

The upstream pull request included such a test (the test had existed
previously in the code base but was disabled and never re-enabled in
the course of porting to a different testing framework).

> Any reason to use OS_CYGWIN_NT and not __CYGWIN__ in the patch ?

Normally I would just use __CYGWIN__ but this is following the
convention used throughout the rest of the OpenBLAS codebase.  It's
actually kind of nice that they have a consistent naming scheme for
platform macros.

On Tue, Feb 6, 2018 at 3:24 PM, Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:
> Also, it should really use pthread functions and drop the notion that
> Cygwin is a Windows target.  Assuming you're running fork from another
> thread than the main thread, does it still work with native Windows
> threads?

I definitely agree in general. I'm going to experiment with a second
patch to just have it use pthreads on Cygwin. I can't think of any
reason it shouldn't work. Is there any particular extra overhead to
pthreads in Cygwin over using the native API directly?  AFAICT the
only slight additional overhead is in thread creation, but that's not
a problem.

I haven't tried forking from another thread. I don't think that should
make an enormous difference, but I'll make a test case to try that.


Best,
E

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple

Re: OpenBLAS patch for Cygwin

[Erik Bray]

On Tue, Feb 6, 2018 at 7:07 PM, Achim Gratz wrote:
> Erik Bray writes:
>> Assuming this looks good (feedback welcome) it might be nice to have
>> this patch included in the next release of the official OpenBLAS
>> package for Cygwin since its incompatibility with fork() has caused
>> problems in the past [1].
>
> It would be vastly preferrable if OPenBLAS ditched the (unfortunately
> quite common) notion that "Cygwin is some sort of Windows" for "Cygwin
> is some sort of Linux".  I've patched out quite some bit of conditionals
> like that in some other packages and it was almost always for the
> better.  The worst ones are those that go into the Windows conditional
> branch and then on to "oh wait, but for Cygwin we need something else".

Yup--OpenBLAS treats Cygwin as "some sort of Windows" and you can see
in my pull requests that there are some conditions like "if WINDOWS &&
!CYGWIN".

I will probably try another patch to remove that notion in general.

I've had a little other experience with patching OpenBLAS for Cygwin
and it's one of those cases where upstream is, fortunately, receptive
to supporting it as a platform.  Their existing Cygwin support has
just been a bit janky (a previous patch I submitted installed the DLL
as cygopenblas.dll instead of libopenblas.dll, for example).

Best,
E

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple