Created attachment 221842 [details] patch update from 1.1 -> 1.2
Build and package info is available at https://gitlab.com/swills/freebsd-ports/pipelines/245879093
Thank you for the submission. As discussed in Phabricator[1] there appears to be some tests. To simply hook-up the tests provided, it would be a case of adding a do-test target as follows into the ports Makefile: do-test: cd ${BUILD_WRKSRC}/test && make bsd run-tests Would you be okay with attaching an updated patch with this change (or I can go ahead and make the amendment on your behalf)? Thanks! [1] https://reviews.freebsd.org/D28307
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.
also, just making sure: the phabricator discussion runs its test of the tests against dyncall 1.1, which is the old version... comes down to the same, though, the tests didn't and still don't have this use-case in mind (but should, as said, will clean this up).
A commit references this bug: Author: lcook Date: Sat Jan 23 21:21:19 UTC 2021 New revision: 562420 URL: https://svnweb.freebsd.org/changeset/ports/562420 Log: devel/dyncall: Update to 1.2 Changes: https://dyncall.org/pub/dyncall/dyncall/file/tip/ChangeLog PR: 252945 Submitted by: Tassilo Philipp <tphilipp@potion-studios.com> (maintainer) Approved by: fernape (mentor) Differential Revision: https://reviews.freebsd.org/D28307 Changes: head/devel/dyncall/Makefile head/devel/dyncall/distinfo head/devel/dyncall/pkg-descr
(In reply to Tassilo Philipp from comment #4) Committed, thanks! Many thanks for the insightful response. These are not things I initially considered nor took into account, so it's refreshing to see your perspective as the author. Please do feel free to CC me <lcook AT FreeBSD.org> in the subsequent update accounting for the issues you mentioned and I'll happily commit the change. Otherwise, thank you for your contributions and great work.
Cool, thanks for understanding. And yeah, more to come, soon, I'm glad you pointed this all out!