Re: checksum argmax fix #2

[Jonathan Perkin <jperkin%mnx.io@localhost>]

* On 2024-08-08 at 07:34 BST, Thomas Klausner wrote:

> On Wed, Aug 07, 2024 at 02:47:22PM +0100, Jonathan Perkin wrote:
> > I've tidied up my previous patch a little, and figured it'd be best to start
> > a new thread.  To make it easier for folks to review and test I've pushed it
> > here:
> >
> >   https://github.com/TritonDataCenter/pkgsrc/commit/3c561ed25e0df2cf8407e4d83bc7369f4ccb7b2a
> >
> > As always just append .patch or .diff to the URL if you want to grab a raw
> > copy, or checkout the dev/distinfo-argmax branch via git.
> >
> > I've removed any mktool bits to simplify things, and changed some of the
> > variables to names that I'm happier with.  Packages now set e.g.
> > DISTINFO_ARGMAX_REQD=524288 to enable the check.
> >
> > This seems to do the right thing in various tests across both NetBSD and
> > macOS, with and without native make, as well as testing with e.g.
> > TOOLS_PLATFORM.mktemp= to ensure it's correctly pulled in as a bootstrap
> > dependency, but I'd be much happier with more widespread testing.
>
> With this patch, but without changing www/grafana to set
> DISTINFO_ARGMAX_REQD, www/grafana now fails with the shell limit
> issue. (Easy to fix, just set DISTINFO_ARGMAX_REQD, but it confused me
> for a second because that one did still work for me before.)

Ah yes, as the default will now be to use inline rather than temporary 
files there may be a couple of DISTINFO_ARGMAX_REQD to add (though maybe 
it is just www/grafana?)  I think it's probably worth it.

> Reading the diff I didn't understand if/where the temporary files all
> get removed, but I didn't notice any remaining in /tmp during my
> tests.

At the end of each target there is:

	@${RM} -f ${_CKSUMFILES_INPUT}

Though I have just noticed that we don't remove it if we take the error 
path in the "checksum" target.  I'll fix that.

> However, pkglint is quite unhappy. The first three happen for every
> package, the last one for grafana if I follow your suggestion above
> (making it += will fix this one).
>
> ERROR: ../../mk/checksum/checksum.mk:39: Assignment modifiers like ":=" must not be used at all.
> ERROR: ../../mk/checksum/checksum.mk:96: Assignment modifiers like ":=" must not be used at all.
> ERROR: ../../mk/checksum/checksum.mk:129: Assignment modifiers like ":=" must not be used at all.
> WARN: Makefile:16: Assignments to "DISTINFO_ARGMAX_REQD" should use "+=", not "=".

Yeh I think we'll need pkglint fixes for these.

Using ::= is the only way this is going to work, so while it shouldn't 
be used in other places, it has to be used here (unless there is some 
other esoteric bmake feature I should be using instead).

And I assume that it treats all _REQD as needing to be lists.  I'm not 
sure I want to change the name of the variable as I think it accurately 
depicts what it does, unless there are any better suggestions?

> So definitely progress, thank you!

Thanks!

--
Jonathan Perkin   -   mnx.io   -   pkgsrc.smartos.org
Open Source Complete Cloud   www.tritondatacenter.com