Bug 261255 - math/nfft: Clean up port (WIP) - Help wanted
Summary: math/nfft: Clean up port (WIP) - Help wanted
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: freebsd-ports-bugs (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-01-16 22:00 UTC by Daniel Engberg
Modified: 2022-03-12 08:06 UTC (History)
3 users (show)

See Also:


Attachments
Patch for nfft (WIP) (88.57 KB, patch)
2022-01-16 22:00 UTC, Daniel Engberg
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Engberg freebsd_committer 2022-01-16 22:00:34 UTC
Created attachment 231058 [details]
Patch for nfft (WIP)

Doing a bit of work fftw3 related ports I noticed that this port pretty much violates all guidelines listed in Porters Handbook so I figured that I should try to get it in better shape.

A few notes, 
The configure script throws errors if you use something else than bash.
The current version pretty much ignores C/LD* flags set and does its own thing (including using -march=native).

It sets "-O3 -fomit-frame-pointer -malign-double -fstrict-aliasing -ffast-math" compared to "-O2 -fstack-protector-strong -isystem /usr/local/include -fno-strict-aliasing". There's obviously a difference but are there any options we should still keep?

It now also throws "ld: error: ../../.libs/libnfft3.so: undefined reference to cexpl" on older versions of FreeBSD (see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=216862) and at least for now I can't figure out how it worked before. My current workaround is to link libopenlibm but I don't think that's a great solution or even a correct one.

A pair of eyes would be appreciated especially regarding the last issue

I've CCed people who've touched this somewhat recently for feedback.
Comment 1 Fernando Apesteguía freebsd_committer 2022-01-19 08:25:28 UTC
(In reply to Daniel Engberg from comment #0)
Hi Daniel,

I built the attached version of the port and I don't see any instances of fomit-frame-pointer in the logs:

/usr/local/poudriere/data/logs/bulk/latest-per-pkg/nfft/3.5.2_2$ grep fomit-frame-pointer *
/usr/local/poudriere/data/logs/bulk/latest-per-pkg/nfft/3.5.2_2$ 

I understand that those are the flags the port uses unconditionally regardless of C/LD FLAGS?

On the issue of cexpl(). It is unfortunate that we don't have it in older versions of FreeBSD. nfft does not provide a "test" target to check for the correctness of the built artifacts, so I would mark the port BROKEN for versions not having cexpl(). math/openlibm seems a mature project, but still, it might lead to very subtle bugs to use different math libraries depending on the FreeBSD version. Since this is a leaf port, this shouldn't cause a cascade of problems.

For curiosity I just did a 

git reset --hard a94f8e0af96a4182e9428d3d02f20c3d8325ecf7

and rebuilt nfft-3.5.1. It builts OK even in 12.2{amd64,i386}. Inspecting the code, cexpl() was already there, but I see that it is checked for its declaration in configure, so maybe somehow its usage was conditional but I can't see how.
Comment 2 Daniel Engberg freebsd_committer 2022-01-19 08:47:00 UTC
Hi,

"-O3 -fomit-frame-pointer -malign-double -fstrict-aliasing -ffast-math" is what the current version in tree sets not using the patch.

The current version in tree doesn't choke on cexpl() but I wonder since we both use amd64 (?) if -march=native somehow defines it? I can't really figure out how this worked before... :/

Best regards,
Daniel
Comment 3 Fernando Apesteguía freebsd_committer 2022-01-19 13:04:23 UTC
(In reply to Daniel Engberg from comment #2)
Thanks, I misunderstood the flags part.

gcc seems to have cexpl() as a builtin function, but I see we use clang now and before with 3.5.1, so that can not be the explanation.
Comment 4 commit-hook freebsd_committer 2022-03-12 08:04:28 UTC
A commit in branch main references this bug:

URL: https://cgit.FreeBSD.org/ports/commit/?id=fa49f6526d6cc24ebdf94471e0a4ae7635297ea6

commit fa49f6526d6cc24ebdf94471e0a4ae7635297ea6
Author:     Daniel Engberg <diizzy@FreeBSD.org>
AuthorDate: 2022-03-12 07:48:21 +0000
Commit:     Daniel Engberg <diizzy@FreeBSD.org>
CommitDate: 2022-03-12 07:48:36 +0000

    math/nfft: Rework port to follow Porters Handbook

    * Use upstream release archive
    * Respect flags set by framework and do not use -march=native
    * Add support for OpenMP and enable it on aarch64 and amd64 by default
    * Overall rework of Makefile
    * Enable all options to match packaging in other distros

    PR:             261255

 math/nfft/Makefile                                |  45 +-
 math/nfft/distinfo                                |   6 +-
 math/nfft/files/cpow.c (gone)                     |  56 --
 math/nfft/files/patch-3rdparty_Makefile.am (gone) |  14 -
 math/nfft/pkg-plist                               | 670 +---------------------
 5 files changed, 36 insertions(+), 755 deletions(-)
Comment 5 Daniel Engberg freebsd_committer 2022-03-12 08:06:03 UTC
Committed with additional changes compared to attched patch