Page MenuHomeFreeBSD

devel/dyncall: Update to 1.2
ClosedPublic

Authored by lcook on Jan 23 2021, 3:35 PM.

Details

Summary

Changelog:

QA:

  • portlint: OK (looks fine).
  • testport: OK (poudriere: 12.2-RELEASE, amd64).

PR: 252945
Submitted by: Tassilo Philipp <tphilipp@potion-studios.com> (maintainer)

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 36432
Build 33321: arc lint + arc unit

Event Timeline

lcook requested review of this revision.Jan 23 2021, 3:35 PM

LGTM.

Good job!

Since this port installs a library we should include as part of the tests, at least one consumer. There are none in this case, so we are good.

As a follow up, the port does include a test suite. We could easily add an option for TESTS and do something like this:

do-test:
        cd ${BUILD_WRKSRC}/test && make
        ${BUILD_WRKSRC}/test/suite/suite
        ${BUILD_WRKSRC}/test/suite2/suite2
        ${BUILD_WRKSRC}/test/suite3/suite3
This revision is now accepted and ready to land.Jan 23 2021, 6:45 PM

LGTM.

Good job!

Since this port installs a library we should include as part of the tests, at least one consumer. There are none in this case, so we are good.

As a follow up, the port does include a test suite. We could easily add an option for TESTS and do something like this:

do-test:
        cd ${BUILD_WRKSRC}/test && make
        ${BUILD_WRKSRC}/test/suite/suite
        ${BUILD_WRKSRC}/test/suite2/suite2
        ${BUILD_WRKSRC}/test/suite3/suite3

Great catch! Having inspected the Makefile.generic there appears to be bsd (links against the -lmmath library) and run-tests targets. Would this be more appropriate:

do-test:
        cd ${BUILD_WRKSRC}/test && make bsd run-tests

Yielding the output:

===>  Testing for dyncall-1.1
cd /usr/ports-trunk/devel/dyncall/work/dyncall-1.1/test && make bsd run-tests
LDLIBS="-lm" make all
make TARGET=all call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_s
elf thunk malloc_wx callback_plain
cd call_suite && make all
cd callback_suite && make all
cd plain && make all
cd plain_c++ && make all
cd suite && make all
cd suite2 && make all
cd suite3 && make all
cd suite_floats && make all
cd ellipsis && make all
cd callf && make all
cd syscall && make all
cd nm && make all
cd dynload_plain && make all
cd resolve_self && make all
cd thunk && make all
cd malloc_wx && make all
cd callback_plain && make all
make TARGET=all call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_self thunk malloc_wx callback_plain
cd call_suite && make all
cd callback_suite && make all
cd plain && make all
cd plain_c++ && make all
cd suite && make all
cd suite2 && make all
cd suite3 && make all
cd suite_floats && make all
cd ellipsis && make all
cd callf && make all
cd syscall && make all
cd nm && make all
cd dynload_plain && make all
cd resolve_self && make all
cd thunk && make all
cd malloc_wx && make all
cd callback_plain && make all
/usr/ports-trunk/devel/dyncall/work/dyncall-1.1/./test/run-build.sh call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_self thunk malloc_wx callback_plain | grep "result:"
result: call_suite: 1
result: callback_suite: 1
result: plain: 1
result: plain_cpp: 1
result: suite: 1
result: suite2: 1
result: suite3: 1
result: suite_floats: 1
result: ellipsis: 1
result: callf: 1
result: syscall: 1
usage : nm/nm <dllpath>
result: dynload_plain: 1
result: resolve_self: 1
result: test_alloc_wx: 1
result: callback_plain: 1

So it seems to be working as expected. Since this is also a submission on bugzilla, would you like me to relay this information back, wait for a newer patch *then* commit?

LGTM.

Good job!

Since this port installs a library we should include as part of the tests, at least one consumer. There are none in this case, so we are good.

As a follow up, the port does include a test suite. We could easily add an option for TESTS and do something like this:

do-test:
        cd ${BUILD_WRKSRC}/test && make
        ${BUILD_WRKSRC}/test/suite/suite
        ${BUILD_WRKSRC}/test/suite2/suite2
        ${BUILD_WRKSRC}/test/suite3/suite3

Great catch! Having inspected the Makefile.generic there appears to be bsd (links against the -lmmath library) and run-tests targets. Would this be more appropriate:

do-test:
        cd ${BUILD_WRKSRC}/test && make bsd run-tests

Yes, I didn't dig that far, just realize there were tests there :-)

Yielding the output:

===>  Testing for dyncall-1.1
cd /usr/ports-trunk/devel/dyncall/work/dyncall-1.1/test && make bsd run-tests
LDLIBS="-lm" make all
make TARGET=all call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_s
elf thunk malloc_wx callback_plain
cd call_suite && make all
cd callback_suite && make all
cd plain && make all
cd plain_c++ && make all
cd suite && make all
cd suite2 && make all
cd suite3 && make all
cd suite_floats && make all
cd ellipsis && make all
cd callf && make all
cd syscall && make all
cd nm && make all
cd dynload_plain && make all
cd resolve_self && make all
cd thunk && make all
cd malloc_wx && make all
cd callback_plain && make all
make TARGET=all call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_self thunk malloc_wx callback_plain
cd call_suite && make all
cd callback_suite && make all
cd plain && make all
cd plain_c++ && make all
cd suite && make all
cd suite2 && make all
cd suite3 && make all
cd suite_floats && make all
cd ellipsis && make all
cd callf && make all
cd syscall && make all
cd nm && make all
cd dynload_plain && make all
cd resolve_self && make all
cd thunk && make all
cd malloc_wx && make all
cd callback_plain && make all
/usr/ports-trunk/devel/dyncall/work/dyncall-1.1/./test/run-build.sh call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_self thunk malloc_wx callback_plain | grep "result:"
result: call_suite: 1
result: callback_suite: 1
result: plain: 1
result: plain_cpp: 1
result: suite: 1
result: suite2: 1
result: suite3: 1
result: suite_floats: 1
result: ellipsis: 1
result: callf: 1
result: syscall: 1
usage : nm/nm <dllpath>
result: dynload_plain: 1
result: resolve_self: 1
result: test_alloc_wx: 1
result: callback_plain: 1

So it seems to be working as expected. Since this is also a submission on bugzilla, would you like me to relay this information back, wait for a newer patch *then* commit?

That would be great. I would upload a new patch to bugzilla with this addition and as a courtesy I would ask the maintainer about it. Also, please update this review with the final version to be committed.

LGTM.

Good job!

Since this port installs a library we should include as part of the tests, at least one consumer. There are none in this case, so we are good.

As a follow up, the port does include a test suite. We could easily add an option for TESTS and do something like this:

do-test:
        cd ${BUILD_WRKSRC}/test && make
        ${BUILD_WRKSRC}/test/suite/suite
        ${BUILD_WRKSRC}/test/suite2/suite2
        ${BUILD_WRKSRC}/test/suite3/suite3

Great catch! Having inspected the Makefile.generic there appears to be bsd (links against the -lmmath library) and run-tests targets. Would this be more appropriate:

do-test:
        cd ${BUILD_WRKSRC}/test && make bsd run-tests

Yes, I didn't dig that far, just realize there were tests there :-)

Yielding the output:

===>  Testing for dyncall-1.1
cd /usr/ports-trunk/devel/dyncall/work/dyncall-1.1/test && make bsd run-tests
LDLIBS="-lm" make all
make TARGET=all call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_s
elf thunk malloc_wx callback_plain
cd call_suite && make all
cd callback_suite && make all
cd plain && make all
cd plain_c++ && make all
cd suite && make all
cd suite2 && make all
cd suite3 && make all
cd suite_floats && make all
cd ellipsis && make all
cd callf && make all
cd syscall && make all
cd nm && make all
cd dynload_plain && make all
cd resolve_self && make all
cd thunk && make all
cd malloc_wx && make all
cd callback_plain && make all
make TARGET=all call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_self thunk malloc_wx callback_plain
cd call_suite && make all
cd callback_suite && make all
cd plain && make all
cd plain_c++ && make all
cd suite && make all
cd suite2 && make all
cd suite3 && make all
cd suite_floats && make all
cd ellipsis && make all
cd callf && make all
cd syscall && make all
cd nm && make all
cd dynload_plain && make all
cd resolve_self && make all
cd thunk && make all
cd malloc_wx && make all
cd callback_plain && make all
/usr/ports-trunk/devel/dyncall/work/dyncall-1.1/./test/run-build.sh call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_self thunk malloc_wx callback_plain | grep "result:"
result: call_suite: 1
result: callback_suite: 1
result: plain: 1
result: plain_cpp: 1
result: suite: 1
result: suite2: 1
result: suite3: 1
result: suite_floats: 1
result: ellipsis: 1
result: callf: 1
result: syscall: 1
usage : nm/nm <dllpath>
result: dynload_plain: 1
result: resolve_self: 1
result: test_alloc_wx: 1
result: callback_plain: 1

So it seems to be working as expected. Since this is also a submission on bugzilla, would you like me to relay this information back, wait for a newer patch *then* commit?

That would be great. I would upload a new patch to bugzilla with this addition and as a courtesy I would ask the maintainer about it. Also, please update this review with the final version to be committed.

OK, just asked the maintainer for some feedback on bugzilla. :-)

LGTM.

Good job!

Since this port installs a library we should include as part of the tests, at least one consumer. There are none in this case, so we are good.

As a follow up, the port does include a test suite. We could easily add an option for TESTS and do something like this:

do-test:
        cd ${BUILD_WRKSRC}/test && make
        ${BUILD_WRKSRC}/test/suite/suite
        ${BUILD_WRKSRC}/test/suite2/suite2
        ${BUILD_WRKSRC}/test/suite3/suite3

Great catch! Having inspected the Makefile.generic there appears to be bsd (links against the -lmmath library) and run-tests targets. Would this be more appropriate:

do-test:
        cd ${BUILD_WRKSRC}/test && make bsd run-tests

Yes, I didn't dig that far, just realize there were tests there :-)

Yielding the output:

===>  Testing for dyncall-1.1
cd /usr/ports-trunk/devel/dyncall/work/dyncall-1.1/test && make bsd run-tests
LDLIBS="-lm" make all
make TARGET=all call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_s
elf thunk malloc_wx callback_plain
cd call_suite && make all
cd callback_suite && make all
cd plain && make all
cd plain_c++ && make all
cd suite && make all
cd suite2 && make all
cd suite3 && make all
cd suite_floats && make all
cd ellipsis && make all
cd callf && make all
cd syscall && make all
cd nm && make all
cd dynload_plain && make all
cd resolve_self && make all
cd thunk && make all
cd malloc_wx && make all
cd callback_plain && make all
make TARGET=all call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_self thunk malloc_wx callback_plain
cd call_suite && make all
cd callback_suite && make all
cd plain && make all
cd plain_c++ && make all
cd suite && make all
cd suite2 && make all
cd suite3 && make all
cd suite_floats && make all
cd ellipsis && make all
cd callf && make all
cd syscall && make all
cd nm && make all
cd dynload_plain && make all
cd resolve_self && make all
cd thunk && make all
cd malloc_wx && make all
cd callback_plain && make all
/usr/ports-trunk/devel/dyncall/work/dyncall-1.1/./test/run-build.sh call_suite callback_suite plain plain_c++ suite suite2 suite3 suite_floats ellipsis callf syscall nm dynload_plain resolve_self thunk malloc_wx callback_plain | grep "result:"
result: call_suite: 1
result: callback_suite: 1
result: plain: 1
result: plain_cpp: 1
result: suite: 1
result: suite2: 1
result: suite3: 1
result: suite_floats: 1
result: ellipsis: 1
result: callf: 1
result: syscall: 1
usage : nm/nm <dllpath>
result: dynload_plain: 1
result: resolve_self: 1
result: test_alloc_wx: 1
result: callback_plain: 1

So it seems to be working as expected. Since this is also a submission on bugzilla, would you like me to relay this information back, wait for a newer patch *then* commit?

That would be great. I would upload a new patch to bugzilla with this addition and as a courtesy I would ask the maintainer about it. Also, please update this review with the final version to be committed.

OK, just asked the maintainer for some feedback on bugzilla. :-)

Maintainers response:

Nice surprise to see this discussion, I didn't expect that, so thank you for the suggestion. As of right now, I would advise against it, though - at least for this port upgrade. Trying to sum up my thoughts about this (I'm also the author of dyncall, btw):

- there are arch specific tests, and some that won't work on all platforms (e.g. syscalls, so it would likely create problems on other archs, and the phabricator discussion seems to only focus on amd64); just to be sure: this is intentional as this is for example about rarely used features that aren't implemented on every platform, yet

- some things in the test folder are for manual investigation and not automated (e.g. displaying W^X behaviour on heap and stack)

- given the above two points, targets like run-tests aren't useful in this context (they are rather a quick way of running all, and be looked at manually), so tests would need to be run one by one to specifically cater to the platform in question

- some tests aren't actually reflecting failure through the exit code, at the moment, so it's misleading, e.g. call_suite succeeds though it might fail

- building the tests takes way more time than building the library (in memory constrained systems this can be a problem)


So, that said, I would *love* to add this, as you suggest, I actually didn't know that "do-test" exists. However, to make it work the way it should, I would need to add patches to the port. Given I'm the author of the library, I would rather just do that directly, clean this all up with "do-test" in mind, and then submit a new version.

This is a great feature, and I'm surely motivated to work on this.


So... would you mind submitting this port upgrade as-is, for now?


PS: Just one thing in the discussion, that I think was maybe misunderstood: the "bsd" target of the makefile should not be used, it actually looks like a remainder from very old times (oops), ./configure takes care of that part. It could be used with Makefile.embedded, though. I'll clean that up, also.

So taking that into account, I'll commit the changes as-is. Since this can only be addressed in later updates.

This revision was automatically updated to reflect the committed changes.