Bug 252945 - devel/dyncall: Update to 1.2
Summary: devel/dyncall: Update to 1.2
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Some People
Assignee: Lewis Cook
URL: https://dyncall.org/pub/dyncall/dynca...
Keywords: buildisok
Depends on:
Reported: 2021-01-23 15:09 UTC by Tassilo Philipp
Modified: 2021-01-23 22:22 UTC (History)
1 user (show)

See Also:
tphilipp: maintainer-feedback+

patch (1.38 KB, patch)
2021-01-23 15:09 UTC, Tassilo Philipp
tphilipp: maintainer-approval+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tassilo Philipp 2021-01-23 15:09:25 UTC
Created attachment 221842 [details]

update from 1.1 -> 1.2
Comment 1 Automation User 2021-01-23 15:22:05 UTC
Build and package info is available at https://gitlab.com/swills/freebsd-ports/pipelines/245879093
Comment 2 Lewis Cook freebsd_committer freebsd_triage 2021-01-23 19:19:42 UTC
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:

        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
Comment 3 Tassilo Philipp 2021-01-23 21:05:19 UTC
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.
Comment 4 Tassilo Philipp 2021-01-23 21:17:52 UTC
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).
Comment 5 commit-hook freebsd_committer 2021-01-23 21:21:59 UTC
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

  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

Comment 6 Lewis Cook freebsd_committer freebsd_triage 2021-01-23 21:22:31 UTC
(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.
Comment 7 Tassilo Philipp 2021-01-23 22:22:14 UTC
Cool, thanks for understanding. And yeah, more to come, soon, I'm glad you pointed this all out!