math/frobby: Improve port and adopt again

[salvadore]

Summary

• Reorder variables.
• Simplify port by introducing USES= localbase:ldflags.
• Set ALL_TARGET= # Empty to avoid defining do-build.
• Remove BINARY_ALIAS: patch Makefile instead.
• Always install executable, remove corresponding option, rename EXEDOCS option to DOCS.
• Always install shared library.
• Install static library with INSTALL_DATA and shared library with INSTALL_LIB.
• Improve shared library management by using an upstream patch. This also solves a poudriere warning about libfrobby.so.0 not having a SONAME.
• Remove outdated warnings about failing tests.
• Fix LIBDOCS dependencies.
• Remove pkg-help file, which does not contain any useful information any more.
• Fix pkg-plist.

[gerald]

Looks good (aka approved) modulo the one comment about dynamic libraries.

math/frobby/Makefile

81

Does this now essentially installed two (identical) copies of that library, where previously only once copy was present and the other was a symlink?

Maybe I am missing something, but that would not be ideal.

[salvadore]

Looking at patch-Makefile I think only one of the three libfrobby.so* files is a real library while the other two are are symlinks to it: indeed you can read one $(CXX) but two ln -s. Am I right using INSTALL_LIB both for real libraries and for symlinks?

The shared library patch is one of the reasons I have put thierry as a reviewer. He is the author of this commit on the same topic:
https://svnweb.freebsd.org/ports?view=revision&revision=569109

@thierry: Does it look right to you? Any suggestion?

[salvadore]

math/frobby/Makefile

81

I have checked using poudriere: this strategy brings in three identical copies of the same library instead of one library and two soft links. I am going to fix that.

[salvadore]

This should handle the shared library correctly.

[salvadore]

I have changed the way to apply the patch. Instead of applying it manually, I am now fetching it from upstream.

[diizzy]

math/frobby/Makefile

9

Just add :-p1 to the line above instead

27

This might not be needed by using USES= localbase:ldflags otherwise fix Makefile and submit upstream

30

Please use USES= localbase:ldflags instead

30–31

This option makes little sense as the port is called frobby and not libfrobby, just make it hardcoded

32

I would argue that we should fix the Makefile instead of trying to hack around it

34–35

There's no need to override Mk/bsd.options.desc.mk ?

34–35

Same as above

54

typo, ben --> be

81

You probably want to use ${RLN} here? Debian also takes a slightly different approach, https://salsa.debian.org/math-team/frobby/-/blob/master/debian/rules#L20

[diizzy]

math/frobby/Makefile

2

PORTVERSION --> DISTVERSION and DISTVERSIONPREFIX should be placed before DISTVERSION
Ref: "Example 17. Use of USE_GITHUB with DISTVERSIONPREFIX" in Porters Handbook

[thierry]

It's OK for me with diizzy's remarks.

math/frobby/Makefile

27

Agree, USES= localbase is cleaner.

[salvadore]

It took me some time, but I have finally applied all the suggested changes and even more. Please see the edited review summary for details.

[salvadore]

math/frobby/Makefile

30–31

I think you were refering to the option to not install frobby's executable, isn't it? I removed it and renamed the EXEDOCS option to DOCS.

34–35

I could not find a SHARED_DESC option in Mk/bsd.options.desc.mk, so I had to keep it.

[diizzy]

I noticed a few nitpicks which you can look at later on as these are not in any way showstoppers, it's my fault for not finding those in the first place.

A word of advice, something that tcberner has taught me is to not "overengineer" a port as it usually leads to overly complex Makefiles, potentially confused users and more time consuming to update so try to limit the amount of options even if it might be tempting unless its something that would benefit a lot of users.

You've done a great deal of work improving this port and it looks much better and follows Porters Handbook more closely. Keep up the great work! :-)

math/frobby/Makefile

13

If there's a license file included, add it (it's not a hard requirement but that's what Porters Handbook says) ;-)
See 5.7.1. LICENSE
Feel free to add it next time you update this port

34–35

Ah yes, we "always" ship on shared libraries, static ones however are optional (usually left up to the maintainer to decide) but this can be fixed later (next update)

54

You can probably set ALL_TARGET= # Empty to avoid defining do-build (you can look into this next update)
https://cgit.freebsd.org/ports/tree/benchmarks/stress-ng/Makefile#n17

[salvadore]

Thank you very much for your reviewing diizzy.

As I don't expect frobby to be updated soon, I prefer to bring in all the fixes now.

I realize that the port was very much "overengineered": indeed, the creation of this port was one of my first contributions to the ports tree so I lacked much experience back then. I now understand how hard it can be to maintain complex Makefiles and try to write them as simple and clear as possible.

[salvadore]

math/frobby/Makefile

13

Actually, I did include a LICENSE_FILE right when I created the port:
https://svnweb.freebsd.org/ports/head/math/frobby/Makefile?view=markup&pathrev=496268

It was later removed when I had dropped frobby's maintainership:
https://svnweb.freebsd.org/ports/head/math/frobby/Makefile?r1=569109&r2=569108&pathrev=569109

@thierry: Do you remember why the line was removed? According to GitHub the file still existed in the repository at v0.9.1.

[thierry]

math/frobby/Makefile

13

@salvadore : when the license is GPLv2+ (or any standard license), I think that we do not have to store one more.

How many does return the command find /usr/local/share/licenses -name COPYING | wc -l on your machine?

[salvadore]

math/frobby/Makefile

13

I see what you mean. Actually, the command returned me 0, but find /usr/local/share/licenses -name GPLv2+ | wc -l returned 32.

I don't know what to do about it. The porter's handbook is inconsistent about it: in https://docs.freebsd.org/en/books/porters-handbook/porting-dads/#dads-misc it asks to avoid to include more copies of the GPL license, but examples from https://docs.freebsd.org/en/books/porters-handbook/makefiles/#licenses do the opposite.

I think I prefer to keep it. I don't think GPL copies are so much of a problem with the disk sizes that we have today. But this is not a strong preference, if you have a strong preference for removing it I can remove it.

[gerald]

Regarding storing the actual license file: If we are looking at a vanilla, 100% standard license file (for GPLv2 say) then I strongly recommend to not ship yet another copy and just notice the license as such.

This isn't so much about disk space as bringing a bit more certainty for consumers that this is really exactly that license. I believe that is one of the intentions of the framework (and of course possibly 100s of copies of the same file don't make too much sense).

Not an absolute must from my perspective, but I strong recommendation, if not ask..

[salvadore]

I have removed LICENSE_FILE, at least for now. I will open a bug report or submit a phabricator review to fix the inconsistency in the porter's handbook. Once the inconsistency is fixed, I will adapt all the GPL licensed ports I maintain accordingly.

@gerald: Do I have your mentor approval? You already approved this review, but it was an older revision and many things have changed since. Thanks.

[gerald]

Yes, I meant to note approval earlier, Lorenzo.

And big thanks to diizzy@ for his reviews!