Changelog:
QA:
- portlint: OK (looks fine).
- testport: OK (poudriere: 12.2-RELEASE, amd64).
PR: 252945
Submitted by: Tassilo Philipp <tphilipp@potion-studios.com> (maintainer)
Details
Details
- Reviewers
tcberner fernape - Commits
- rP562420: devel/dyncall: Update to 1.2
Diff Detail
Diff Detail
- Repository
- rP FreeBSD ports repository
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 36432 Build 33321: arc lint + arc unit
Event Timeline
Comment Actions
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/suite3Comment Actions
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-testsYielding 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?
Comment Actions
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: 1So 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.
Comment Actions
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.