Re: OpenBLAS patch for Cygwin
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
Re: OpenBLAS patch for Cygwin
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 Vinschenwrote: > 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 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
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 signature.asc Description: PGP signature
Re: OpenBLAS patch for Cygwin
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