databases/ruby-bdb: Allow building as a user on ZFS

[loader]

Summary

Unit tests log (original)
https://bz-attachments.freebsd.org/attachment.cgi?id=193388

Unit tests log (with patches), there are still a few tests failed, haven't figured out yet.
https://bz-attachments.freebsd.org/attachment.cgi?id=193389

Proposed commit log message:

databases/ruby-bdb: Allow building as a user on ZFS

While I'm here,

- Fix a few failed unit tests
- Add TEST_DEPENDS devel/rubygem-test-unit
- Remove -lunwind option for overlinking
- Remove custom regression-test target (in favour of source patch)
- Pet portlint(1): USE_RUBY before USES
  PLIST_FILES contains %%RUBY_SITEARCHLIBDIR%% variable
  regenerate patch-src-cursor.c and patch-src-recnum.c with make makepatch

PR: 228023
Reviewed_by: knu, koobs, mat, swills
Approved by: knu (maintainer), koobs (mentor), mat, swills
Differential_Revision: D15425
MFH: 2018Q2

Test Plan

• portlint: OK (

WARN: Makefile: [11]: possible direct use of command "ruby" found. use ${RUBY} instead.
WARN: Makefile: [68]: possible use of "${CHMOD}" found. Use @(owner,group,mode) syntax or @owner/@group operators in pkg-plist instead.
0 fatal errors and 2 warnings found.)

• testport: OK (poudriere: 1200062, [amd64, armv7, aarch64], '', ruby-2.4.3,1 tested)

• unittest: OK (

armv7:
tests/recnum.rb:
17 tests, 2398 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/hash.rb:
22 tests, 15761 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/btree.rb:
21 tests, 15798 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/lock.rb:
10 tests, 172 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/log:
12 tests, 2315 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/queue:
17 tests, 130 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/recno:
24 tests, 157 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/marshal.rb:
18 tests, 2944 assertions, 1 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
94.4444% passed

amd64, aarch64:
tests/recnum.rb:
17 tests, 2398 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/hash.rb:
22 tests, 7657 assertions, 0 failures, 3 errors, 0 pendings, 0 omissions, 0 notifications
86.3636% passed
tests/btree.rb:
21 tests, 15780 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/lock.rb:
10 tests, 172 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/log:
12 tests, 2315 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/queue:
17 tests, 130 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/recno:
24 tests, 157 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
tests/marshal.rb:
18 tests, 2944 assertions, 1 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
94.4444% passed)

[loader]

databases/ruby-bdb/files/patch-src_common.c

55

This change fixes tests/btree.rb (test_23_sh), tests/marshal.rb (test_17_sh) and tests/harsh.rb (test_21_sh).

rb_method_boundp() returns 0 or 1, but Qtrue is 0x14 in ruby/ruby.h.

https://svn.ruby-lang.org/cgi-bin/viewvc.cgi/tags/v2_4_4/vm_method.c?revision=63013&view=markup#l1092

https://svn.ruby-lang.org/cgi-bin/viewvc.cgi/tags/v2_4_4/include/ruby/ruby.h?revision=63013&view=markup#l406

406     #if USE_FLONUM
407         RUBY_Qfalse = 0x00,         /* ...0000 0000 */
408         RUBY_Qtrue  = 0x14,         /* ...0001 0100 */
409         RUBY_Qnil   = 0x08,         /* ...0000 1000 */
410         RUBY_Qundef = 0x34,         /* ...0011 0100 */

68

This change is to fix the unit tests errros like

ruby24 tests/btree.rb
                                                                                              
VERSION of BDB is Berkeley DB 5.3.28: (September  9, 2013)                                    
Loaded suite tests/btree                                
Started                                
............/usr/ports/databases/ruby-bdb/work/bdb-0.6.6/src/bdb.so: Undefined symbol "RBASIC_SET_CLASS_RAW"

https://svn.ruby-lang.org/cgi-bin/viewvc.cgi/tags/v2_4_4/doc/extension.rdoc?revision=63013&view=markup#l1727

1727    === Incompatibility
1728    
1729    You can't write RBASIC(obj)->klass field directly because it is const
1730    value now.
1731    
1732    Basically you should not write this field because MRI expects it to be
1733    an immutable field, but if you want to do it in your extension you can
1734    use the following functions:
1735    
1736    VALUE rb_obj_hide(VALUE obj) ::
1737    
1738      Clear RBasic::klass field. The object will be an internal object.
1739      ObjectSpace::each_object can't find this object.
1740        
1741    VALUE rb_obj_reveal(VALUE obj, VALUE klass) ::
1742        
1743      Reset RBasic::klass to be klass.
1744      We expect the `klass' is hidden class by rb_obj_hide().

98

use Module#<= to check the class ancestors instead of using RCLASS_M_TBL() directly.
this fixes a few Undefined symbol "RCLASS_M_TBL" errors:

ruby24 tests/recno.rb

VERSION of BDB is Berkeley DB 5.3.28: (September  9, 2013)
Loaded suite tests/recno
Started
/usr/ports/databases/ruby-bdb/work/bdb-0.6.6/src/bdb.so: Undefined symbol "RCLASS_M_TBL"


ruby24 tests/queue.rb

VERSION of BDB is Berkeley DB 5.3.28: (September  9, 2013)
Loaded suite tests/queue
Started
/usr/ports/databases/ruby-bdb/work/bdb-0.6.6/src/bdb.so: Undefined symbol "RCLASS_M_TBL"

[koobs]

Bump PORTREVISION does not sufficiently describe the summary of changes. Since this is related to Issue 228023, use it's summary as the shortlog

Are the patches going upstream? If so, add comments to patch headers with upstream link references, if any. If not, mention that and why not in the commit log

databases/ruby-bdb/Makefile

72–74

Is this really an unconditional LIB_DEPENDS? If so, was it a missing (necessary) dependency? If so, does it need to be merged?

The change is not explained in the commit log message

89–90

Can this be replaced with TEST_TARGET and TEST_WRKSRC or does the creation of the /tmp dir preclude it? Does the test fail without us creating the directory?

Perhaps patching/upstreaming the test target of the upstream sources to do it (the dir creation) is a better (long-term) way to go. That way this whole block can go.

[loader]

databases/ruby-bdb/Makefile

74

Because lang/ruby24 already links with libunwind on i386 and amd64:
https://svnweb.freebsd.org/ports/head/lang/ruby24/Makefile?revision=462120&view=markup#l94

94 	# keep in sync with all platforms where libunwind is available
95 	.if (${ARCH} == amd64 || ${ARCH} == i386)
96 	LIB_DEPENDS+=   libunwind.so:devel/libunwind
97 	.endif

Then bdb.so links with libunwind.so.8 on i386 and amd64 too.

Error: /usr/local/lib/ruby/site_ruby/2.4/amd64-freebsd12/bdb.so is linked to /usr/local/lib/libunwind.so.8 from devel/libunwind but it is not declared as a dependency                        
Warning: you need LIB_DEPENDS+=libunwind.so:devel/libunwind

89–90

sure, I will upload a new patch.

The port maintainer (knu@) is also the author of this Ruby module,
but it looks like this hasn't been updated for years.
https://github.com/knu/ruby-bdb

[loader]

remote the regression-test targe

[loader]

In D15425#325266, @koobs wrote:

Bump PORTREVISION does not sufficiently describe the summary of changes. Since this is related to Issue 228023, use it's summary as the shortlog

Sure, I've updated it.

Are the patches going upstream? If so, add comments to patch headers with upstream link references, if any. If not, mention that and why not in the commit log

Okay, I will create a pull request and add the link the patch header.

databases/ruby-bdb/Makefile

89–90

Can this be replaced with TEST_TARGET and TEST_WRKSRC or does the creation of the /tmp dir preclude it?

Thanks koobs@. Yes it's a better way to go.
I just added TEST_TARGET and moved mkdir tmp to ${WRKSRC}/extconf.rb.

Does the test fail without us creating the directory?

Yes, Unit tests write temporary db files into this tmp/ directory, they will fail without it.

[loader]

Add upstream pull request link to files/patch-src_common.c

[koobs]

LGTM, if this change needs to be merged (so that the quarterly version can be built without root), please add relevant MFH: YYYYQX line to commit log message

[loader]

In D15425#325629, @koobs wrote:

LGTM, if this change needs to be merged (so that the quarterly version can be built without root), please add relevant MFH: YYYYQX line to commit log message

Thank you @koobs for all your help. :D
Should I wait a bit longer let @knu or @swills review the patch before I commit it?

[koobs]

In D15425#325670, @loader wrote:

In D15425#325629, @koobs wrote:

LGTM, if this change needs to be merged (so that the quarterly version can be built without root), please add relevant MFH: YYYYQX line to commit log message

Thank you @koobs for all your help. :D
Should I wait a bit longer let @knu or @swills review the patch before I commit it?

Apart from mentor approval (when being mentored), all other approval types still apply. In this case:

1. ports with a maintainer: maintainer approval, or maintainer timeout (>= 2 weeks after the date of an initial patch is provided, or >=2 weeks after an updated patch with new/major changes is provided)
2. changes to be merged: merge approval (implicit for blanket changes, or explicit ports-secteam for other changes)

If there are ruby-related doubts/questions, its prudent to get someone with ruby expertise (or from the ruby team, ala swills)

[mat]

I think by:

databases/ruby-bdb: Allow poudriere running with BUILD_AS_NON_ROOT=yes on ZFS

you mean:

databases/ruby-bdb: Allow building as a user on ZFS

poudriere as nothing to do with it.

databases/ruby-bdb/Makefile

21

Out of order. See Chapter 15. Order of Variables in Port Makefiles.

60–68

This seems like the wrong fix, please do not commit, and let me have a look at it first.

72–74

This is probably a case of overlinking, I do not think it is required. Are you sure that bdb.so uses any symbols from that library?

[mat]

As I cannot reproduce, I definitively think this is the wrong fix for another problem.

[loader]

update for OPTIONS_DEFINE variable order and remove -lunwind overlinking

[loader]

databases/ruby-bdb/Makefile

72–74

Yes, it's overlinking. The -lunwind comes from the CONFIG["LIBS"] in %%RUBY_ARCHLIBDIR%%/rbconfig.rb

I added a post-configure target to remove it.

[loader]

spaces => tab

[koobs]

@loader

If this is to proceed, it requires either maintainer approval, or a maintainer timeout (>2 weeks) approval. The latter case requires a Bugzilla issue. Since there's hasn't been feedback for the first case here as yet, please create a Bugzilla issue with a link to this review (but also include a patch as attachment there too) so it can timeout if necessary

[loader]

Hi @koobs,

There's a Bugzilla issue here:
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=228023
https://github.com/knu/ruby-bdb/pull/6

Since the default version of Ruby has been updated to 2.5 and this only occurs on ZFS with USE_TMPFS=no in poudriere.conf, I'm not sure whether the patch is still needed, I'll just go head to close this review.

Thanks.