Bug 241974 - lang/nim: Remove BROKEN for powerpc64
Summary: lang/nim: Remove BROKEN for powerpc64
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: powerpc Any
: --- Affects Some People
Assignee: Piotr Kubaj
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-11-14 23:33 UTC by Curtis Hamilton
Modified: 2019-11-27 19:55 UTC (History)
4 users (show)

See Also:
ports: maintainer-feedback+


Attachments
Patch to fix build on powerpc64 (2.40 KB, patch)
2019-11-18 17:49 UTC, Curtis Hamilton
no flags Details | Diff
Updated patch to fix build on powerpc64 (1.02 KB, patch)
2019-11-23 20:04 UTC, Curtis Hamilton
no flags Details | Diff
Updated patch to fix build on powerpc64 (1.20 KB, patch)
2019-11-23 21:25 UTC, Curtis Hamilton
no flags Details | Diff
Revised patch (1.25 KB, patch)
2019-11-25 14:33 UTC, Curtis Hamilton
no flags Details | Diff
Revised patch (1.22 KB, patch)
2019-11-25 14:41 UTC, Curtis Hamilton
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Curtis Hamilton 2019-11-14 23:33:42 UTC
Nim support for powerpc64 has been added.

Ref: https://github.com/nim-lang/Nim/pull/12011
Comment 1 Neal Nelson 2019-11-15 07:11:11 UTC
Has this been tested and shown to work? I don't have a powerpc64 system so can't test it.

If so, please submit a patch.
Comment 2 Curtis Hamilton 2019-11-16 05:03:32 UTC
(In reply to Neal Nelson from comment #1)
No patch is required. I’m able to build the port package after removing the “BROKEN_powerpc64” line in the makefile.

However, I’ll ask someone else who has a PowerPC64 system to try building the package.
Comment 3 Piotr Kubaj freebsd_committer 2019-11-17 17:34:43 UTC
Still doesn't build on 12.1-RELEASE:
===>  Building for nim-1.0.2
cd /usr/local/poudriere/ports/default/lang/nim/work/nim-1.0.2 && /usr/bin/env CC="cc" LINKER="cc"  COMP_FLAGS=" -O2 -pipe  -fstack-protector-strong -fno-strict-aliasing " LINK_FLAGS=
" -fstack-protector-strong "  /bin/sh build.sh
# OS: freebsd
# CPU: powerpc64
clang -w -O3 -fno-strict-aliasing -m64 -Ic_code -c c_code/1_2/stdlib_assertions.nim.c -o c_code/1_2/stdlib_assertions.nim.o
build.sh: clang: not found

It builds on when using clang in base but this is currently only on head and requires additional patches - official head still uses elfv1.
Comment 4 Curtis Hamilton 2019-11-18 17:49:09 UTC
Created attachment 209232 [details]
Patch to fix build on powerpc64

Thanks for the feedback!  

I completely forgot about testing on 12.x-RELEASE.  The attached patch should fix builds on releases prior to 13-RELEASE.
Comment 5 Piotr Kubaj freebsd_committer 2019-11-19 08:54:23 UTC
Testing. Note that even head uses GCC for now.
Comment 6 Piotr Kubaj freebsd_committer 2019-11-19 09:21:01 UTC
Builds now on 12.1-RELEASE. I will commit your patch when the maintainer accepts it.
Comment 7 Neal Nelson 2019-11-19 09:27:21 UTC
Comment on attachment 209232 [details]
Patch to fix build on powerpc64

The patch looks OK to me.
Comment 8 Piotr Kubaj freebsd_committer 2019-11-20 09:56:43 UTC
Now it won't build on ELFv2:
===>  Building for nim-1.0.2
cd /tmp/usr/ports/lang/nim/work/nim-1.0.2 && /usr/bin/env CC="cc" LINKER="cc"  COMP_FLAGS=" -O2 -pipe  -fstack-protector-strong -fno-strict-aliasing " LINK_FLAGS=" -fstack-protector-strong "  /bin/sh build.sh
# OS: freebsd
# CPU: powerpc64
gcc -w -O3 -fno-strict-aliasing -m64 -Ic_code -c c_code/1_2/stdlib_assertions.nim.c -o c_code/1_2/stdlib_assertions.nim.o
build.sh: gcc: not found

You need to add those patches to EXTRA_PATCHES and make them apply only when GCC is used. See PPC_ABI variable.
Comment 9 Curtis Hamilton 2019-11-23 20:04:09 UTC
Created attachment 209368 [details]
Updated patch to fix build on powerpc64

Updated patch to check PPC_ABI.
Comment 10 Curtis Hamilton 2019-11-23 21:25:35 UTC
Created attachment 209370 [details]
Updated patch to fix build on powerpc64
Comment 11 Piotr Kubaj freebsd_committer 2019-11-24 12:28:27 UTC
There are still several issues.

There is another include of bsd.port.pre.mk, which itself breaks build.

It will be much cleaner to do it like:
.if defined(PPC_ABI) && ${PPC_ABI} == ELFv2
EXTRA_PATCHES=${PATCHDIR}/elfv2-patch
.else
EXTRA_PATCHES=${PATCHDIR}/elfv1-patch
.endif

Such approach is clean and makes it easy to see what you do.

In post-patch, you check for PPC_ABI, but this is only defined on powerpc64, which will break build on anything else.
You could check like:
.if defined(PPC_ABI) && ${PPC_ABI} == ELFv2 (or 1)
Comment 12 Curtis Hamilton 2019-11-25 14:33:47 UTC
Created attachment 209410 [details]
Revised patch

Updated patch to reflect recommendations
Comment 13 Curtis Hamilton 2019-11-25 14:41:03 UTC
Created attachment 209411 [details]
Revised patch
Comment 14 Piotr Kubaj freebsd_committer 2019-11-26 15:35:33 UTC
(In reply to Curtis Hamilton from comment #13)
It now builds on powerpc64 (both elfv1 and elfv2) and amd64.

Once maintainer approves it, I'll post a review so that my mentors can approve it.
Comment 15 Neal Nelson 2019-11-27 09:54:15 UTC
I've tried to set the maintainer flags to approved, but failing that here's my go ahead anyway.
Comment 16 commit-hook freebsd_committer 2019-11-27 19:55:37 UTC
A commit references this bug:

Author: pkubaj
Date: Wed Nov 27 19:55:14 UTC 2019
New revision: 518529
URL: https://svnweb.freebsd.org/changeset/ports/518529

Log:
  lang/nim: Remove BROKEN for powerpc64

  Fix build on powerpc64 elfv1 and elfv2.

  PR:             241974
  Submitted by:   hamiltcl@verizon.net
  Approved by:    ports@nicandneal.net (maintainer), tcberner (mentor)
  Differential Revision:  https://reviews.freebsd.org/D22568

Changes:
  head/lang/nim/Makefile
  head/lang/nim/files/elfv1-patch-build.sh