Bug 206886

Summary: math/openblas: Update to 0.2.15
Product: Ports & Packages Reporter: groot
Component: Individual Port(s)Assignee: Raphael Kubo da Costa <rakuco>
Status: Closed FIXED    
Severity: Affects Some People CC: phd_kimberlite, rakuco
Priority: --- Keywords: needs-qa, patch
Version: LatestFlags: phd_kimberlite: maintainer-feedback+
rakuco: merge-quarterly+
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Update Makefile and distinfo
none
Patch build (rlimits)
none
patch including minor collection
phd_kimberlite: maintainer-approval+
patch for comment #6
phd_kimberlite: maintainer-approval+
patch for comment #8 phd_kimberlite: maintainer-approval+

Description groot 2016-02-03 16:49:34 UTC
math/openblas does some compile-time CPU detection. That's probably not a good idea from a reproducible-builds perspective anyway, but I can't find an option to turn it off *and* the detection in v0.2.14 doesn't detect Skylake.

0.2.15 from october 2015 -- http://www.openblas.net/Changelog.txt -- fixes the detection.

(affects kde@ because it's a dependency for some of our ports use openblas, and I build on a Skylake machine nowadays)
Comment 1 groot 2016-02-03 16:51:23 UTC
Created attachment 166506 [details]
Update Makefile and distinfo

Needs more patches than this, because of compile error with struct rlimit.
Comment 2 Kubilay Kocak freebsd_committer freebsd_triage 2016-02-03 16:56:31 UTC
(In reply to groot from comment #1)

Will you be providing said patches?
Comment 3 groot 2016-02-03 18:40:58 UTC
Created attachment 166520 [details]
Patch build (rlimits)

This patch adds a patch to files/ that fixes the build; without it, getrlimit(2) and related structures are undefined in blas_server.c
Comment 4 Kubilay Kocak freebsd_committer freebsd_triage 2016-02-04 07:10:01 UTC
Thank you Adriaan :)

Could you combine these patches into a single unified diff against the port HEAD, after creating said files/patch-* with `make makepatch` please?

Alternatively, can/should any of these patches be merge back to the quarterly branch? You mention fixing the build, is that relevant to the current version, or only after the version update provided here?

Also, confirmation of QA (portlint, poudriere) passing would help progress the issue.
Comment 5 Eijiro Shibusawa 2016-02-04 14:19:21 UTC
Created attachment 166560 [details]
patch including minor collection

Hi,

The attached is the patch to HEAD including minor collection.
It is tested by using portlint 2.16.8 and poudriere 3.1.10.
Comment 6 Raphael Kubo da Costa freebsd_committer 2016-02-05 10:13:12 UTC
Hi,

The patch from comment #5 fails to build on 9.3 because base's GCC is too old:

memory.c:1341: error: wrong number of arguments specified for 'constructor' attribute
memory.c:1384: error: wrong number of arguments specified for 'destructor' attribute

Line 1341 is:
void CONSTRUCTOR gotoblas_init(void) {

And the failure comes from the fact that CONSTRUCTOR is being defined like this:
#if defined(_MSC_VER) && !defined(__clang__)
#define CONSTRUCTOR __cdecl
#define DESTRUCTOR __cdecl
#elif defined(OS_DARWIN) && defined(C_GCC)
#define CONSTRUCTOR     __attribute__ ((constructor))
#define DESTRUCTOR      __attribute__ ((destructor))
#else
#define CONSTRUCTOR     __attribute__ ((constructor(101)))
#define DESTRUCTOR      __attribute__ ((destructor(101)))
#endif

GCC 4.2.1 does not understand the constructor(priority) syntax, which is likely why there's an OS X-specific definition before that.

You need to either add a patch to use that one when an old GCC is being used (or just check for __FreeBSD_version) or require a more recent compiler.
Comment 7 Eijiro Shibusawa 2016-02-06 12:50:53 UTC
Created attachment 166656 [details]
patch for comment #6

Hi,

I've created an additional patch.
The attached, at least, passed poudriere test (9.3R and 10.2R) on amd64,
however, it may not be valid solution.
On the test for 10.2 I used GCC 4.8 and clang 3.4.1.
Comment 8 Raphael Kubo da Costa freebsd_committer 2016-02-07 13:44:20 UTC
The new patch may risk introducing the bug that the commit adding priorities to the constructor and destructor attributes tried to fix (https://github.com/xianyi/openblas/issues/654) even for GCC versions which do support that feature (basically anything newer than base GCC).

How about this instead:

--- driver/others/memory.c
+++ driver/others/memory.c
@@ -142,7 +142,7 @@ USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #if defined(_MSC_VER) && !defined(__clang__)
 #define CONSTRUCTOR __cdecl
 #define DESTRUCTOR __cdecl
-#elif defined(OS_DARWIN) && defined(C_GCC)
+#elif defined(OS_DARWIN) || (defined(C_GCC) && ((__GNUC__ == 4) && (__GNUC_MINOR__ < 3)))
 #define CONSTRUCTOR	__attribute__ ((constructor))
 #define DESTRUCTOR	__attribute__ ((destructor))
 #else
Comment 9 Raphael Kubo da Costa freebsd_committer 2016-02-15 15:03:38 UTC
Ping, everyone.
Comment 10 Eijiro Shibusawa 2016-02-18 15:05:59 UTC
Created attachment 167152 [details]
patch for comment #8
Comment 11 commit-hook freebsd_committer 2016-02-18 16:36:31 UTC
A commit references this bug:

Author: rakuco
Date: Thu Feb 18 16:35:50 UTC 2016
New revision: 409114
URL: https://svnweb.freebsd.org/changeset/ports/409114

Log:
  Update to 0.2.15.

  0.2.15 was released in October 2015.
  Release notes: http://www.openblas.net/Changelog.txt

  This update introduces support for new CPU architectures, such as Intel's
  Broadwell and Skylake. In practice, this means people using those architectures
  can actually build the port now (OpenBLAS seems to do some CPU-detection that
  cannot be easily turned off and refusing to build on unrecognized CPUs).

  Port changes:
  - Reorganize a few variables in Makefile.
  - Refresh patches.

  PR:		206886
  Submitted by:	Adriaan de Groot <groot@kde.org> (first version),
  		Eijiro Shibusawa <phd_kimberlite@yahoo.co.jp> (maintainer)
  MFH:		2016Q1

Changes:
  head/math/openblas/Makefile
  head/math/openblas/distinfo
  head/math/openblas/files/patch-Makefile
  head/math/openblas/files/patch-Makefile.rule
  head/math/openblas/files/patch-Makefile.system
  head/math/openblas/files/patch-c_check
  head/math/openblas/files/patch-cpuid_ia64.c
  head/math/openblas/files/patch-cpuid_sparc.c
  head/math/openblas/files/patch-driver_others_blas__server.c
  head/math/openblas/files/patch-driver_others_memory.c
  head/math/openblas/files/patch-exports+Makefile
  head/math/openblas/files/patch-f_check
Comment 12 commit-hook freebsd_committer 2016-02-18 18:43:44 UTC
A commit references this bug:

Author: rakuco
Date: Thu Feb 18 18:43:26 UTC 2016
New revision: 409123
URL: https://svnweb.freebsd.org/changeset/ports/409123

Log:
  MFH: r409114

  Update to 0.2.15.

  0.2.15 was released in October 2015.
  Release notes: http://www.openblas.net/Changelog.txt

  This update introduces support for new CPU architectures, such as Intel's
  Broadwell and Skylake. In practice, this means people using those architectures
  can actually build the port now (OpenBLAS seems to do some CPU-detection that
  cannot be easily turned off and refusing to build on unrecognized CPUs).

  Port changes:
  - Reorganize a few variables in Makefile.
  - Refresh patches.

  PR:		206886
  Submitted by:	Adriaan de Groot <groot@kde.org> (first version),
  		Eijiro Shibusawa <phd_kimberlite@yahoo.co.jp> (maintainer)

  Approved by:	ports-secteam (feld)

Changes:
_U  branches/2016Q1/
  branches/2016Q1/math/openblas/Makefile
  branches/2016Q1/math/openblas/distinfo
  branches/2016Q1/math/openblas/files/patch-Makefile
  branches/2016Q1/math/openblas/files/patch-Makefile.rule
  branches/2016Q1/math/openblas/files/patch-Makefile.system
  branches/2016Q1/math/openblas/files/patch-c_check
  branches/2016Q1/math/openblas/files/patch-cpuid_ia64.c
  branches/2016Q1/math/openblas/files/patch-cpuid_sparc.c
  branches/2016Q1/math/openblas/files/patch-driver_others_blas__server.c
  branches/2016Q1/math/openblas/files/patch-driver_others_memory.c
  branches/2016Q1/math/openblas/files/patch-exports+Makefile
  branches/2016Q1/math/openblas/files/patch-f_check
Comment 13 Raphael Kubo da Costa freebsd_committer 2016-02-18 18:44:16 UTC
Finally landed and backported. Thanks, everyone!