Page MenuHomeFreeBSD

math/frobby: Improve port and adopt again
ClosedPublic

Authored by salvadore on Apr 21 2022, 10:37 PM.
Tags
None
Referenced Files
F81803217: D35028.diff
Sun, Apr 21, 5:58 PM
Unknown Object (File)
Mar 20 2024, 4:20 PM
Unknown Object (File)
Feb 12 2024, 10:00 PM
Unknown Object (File)
Feb 6 2024, 7:06 AM
Unknown Object (File)
Jan 28 2024, 2:53 PM
Unknown Object (File)
Jan 28 2024, 2:53 PM
Unknown Object (File)
Jan 28 2024, 2:53 PM
Unknown Object (File)
Jan 28 2024, 2:53 PM
Subscribers

Details

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.

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 45374
Build 42262: arc lint + arc unit

Event Timeline

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

math/frobby/Makefile
94

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.

This revision is now accepted and ready to land.Apr 24 2022, 2:37 PM

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?

math/frobby/Makefile
94

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.

This should handle the shared library correctly.

This revision now requires review to proceed.Apr 26 2022, 12:49 AM

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

diizzy added inline comments.
math/frobby/Makefile
9

Just add :-p1 to the line above instead

24

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

27

Please use USES= localbase:ldflags instead

29

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

31

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

39

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

40–41

Same as above

58

typo, ben --> be

94

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

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 requested changes to this revision.EditedApr 28 2022, 5:00 PM

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

math/frobby/Makefile
24

Agree, USES= localbase is cleaner.

This revision now requires changes to proceed.Apr 28 2022, 5:00 PM

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 edited the summary of this revision. (Show Details)
salvadore added inline comments.
math/frobby/Makefile
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.

39

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

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
10

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

39

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)

65

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

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 added inline comments.
math/frobby/Makefile
10

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 added inline comments.
math/frobby/Makefile
10

@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?

This revision is now accepted and ready to land.May 4 2022, 4:39 PM
salvadore added inline comments.
math/frobby/Makefile
10

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.

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..

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.

This revision now requires review to proceed.May 4 2022, 9:08 PM

Yes, I meant to note approval earlier, Lorenzo.

And big thanks to diizzy@ for his reviews!

This revision is now accepted and ready to land.May 5 2022, 5:37 AM