Bug 244239 - ports-mgmt/portlint: strange output for security/p5-IO-Socket-SSL
Summary: ports-mgmt/portlint: strange output for security/p5-IO-Socket-SSL
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: Port Management Team
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-02-20 08:33 UTC by Kurt Jaeger
Modified: 2020-12-27 23:57 UTC (History)
5 users (show)

See Also:
koobs: merge-quarterly?


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kurt Jaeger freebsd_committer 2020-02-20 08:33:58 UTC
This looks wrong:

$ cd security/p5-IO-Socket-SSL
$ portlint -AC
WARN: Makefile: for new port, make $FreeBSD$ tag in comment section empty, to make SVN happy.
WARN: Makefile: no port directory /usr/ports/UIDN}_${ found, even though it is listed in BUILD_DEPENDS.
WARN: Makefile: no port directory /usr/ports/UIPV6}_${ found, even though it is listed in BUILD_DEPENDS.

Any ideas on how to fix this ?
Comment 1 Mikael Urankar freebsd_committer 2020-02-20 10:20:35 UTC
(In reply to Kurt Jaeger from comment #0)
reverse the order of RUN_DEPENDS and BUILD_DEPENDS:

--- Makefile    (revision 526484)
+++ Makefile    (working copy)
@@ -14,9 +14,9 @@
 LICENSE_COMB=  dual
 LICENSE_FILE=  ${WRKSRC}/lib/IO/Socket/SSL.pod
 
-BUILD_DEPENDS= ${RUN_DEPENDS}
 RUN_DEPENDS=   p5-Net-SSLeay>=1.59:security/p5-Net-SSLeay \
                p5-Mozilla-CA>=20130114:www/p5-Mozilla-CA
+BUILD_DEPENDS= ${RUN_DEPENDS}
 
 NO_ARCH=       yes
 USES=          perl5 shebangfix



portlint -AC
WARN: Makefile: for new port, make $FreeBSD$ tag in comment section empty, to make SVN happy.
WARN: Consider to set DEVELOPER=yes in /etc/make.conf
0 fatal errors and 2 warnings found.
Comment 3 Sergei Vyshenski 2020-05-03 00:41:17 UTC
Hi,

A note for discussion.

Subroutine of /usr/local/bin/portlint named
sub get_makevar_shallow {}
forms argument list for "make" as follows (see line 3821):

$cmd = join(' -dV -V ', "make $makeenv MASTER_SITE_BACKUP=''", map { "'$_'" } @_);

which causes exactly reported garbage:

$ cd /usr/ports/security/p5-IO-Socket-SSL
$ make MASTER_SITE_BACKUP='' -dV -V 'RUN_DEPENDS'
p5-Net-SSLeay>=1.59:security/p5-Net-SSLeay  p5-Mozilla-CA>=20130114:www/p5-Mozilla-CA ${${:UIDN}_${:URUN}_DEPENDS} ${${:UIPV6}_${:URUN}_DEPENDS} ${PERL5_DEPEND}:lang/${PERL_PORT}
$ portling -AC
WARN: Makefile: for new port, make $FreeBSD$ tag in comment section empty, to make SVN happy.
WARN: Makefile: no port directory /usr/ports/UIPV6}_${ found, even though it is listed in BUILD_DEPENDS.
0 fatal errors and 2 warnings found.

The "-dV" flag for make suppresses variable expansion, and seems irrelevant for the ports with conditional dependency list. 
If this flag is omitted from line 3821 of /usr/local/bin/portlint, that is:

$cmd = join(' -V ', "make $makeenv MASTER_SITE_BACKUP=''", map { "'$_'" } @_);

the garbage is gone:

$ cd /usr/ports/security/p5-IO-Socket-SSL
$ make MASTER_SITE_BACKUP='' -V 'RUN_DEPENDS'
p5-Net-SSLeay>=1.59:security/p5-Net-SSLeay  p5-Mozilla-CA>=20130114:www/p5-Mozilla-CA p5-URI>=1.50:net/p5-URI p5-IO-Socket-INET6>0:net/p5-IO-Socket-INET6 perl5>=5.30.r1<5.31:lang/perl5.30
$ portling -AC
WARN: Makefile: for new port, make $FreeBSD$ tag in comment section empty, to make SVN happy.
0 fatal errors and 1 warning found.

Sorry for slow thinking, and not sure though if the proposed change fits for the rest of the world.

Regards, Sergei
Comment 4 Joe Marcus Clarke freebsd_committer 2020-05-04 17:22:37 UTC
Can you try this patch instead:

https://www.marcuscom.com/~jclarke/portlint.pl.diff
Comment 5 Sergei Vyshenski 2020-05-04 21:54:43 UTC
Markus,

Yes, at first it complains as: 
FATAL: Makefile: BUILD_DEPENDS points to RUN_DEPENDS which has not yet been defined.
And after changing order of BUILD_ and RUN_ lines, it becomes happy.
Which violates point 15.6. "The Dependencies Block" of Porter's Handbook.

The variables are:
    FETCH_DEPENDS
    EXTRACT_DEPENDS
    PATCH_DEPENDS
    BUILD_DEPENDS
    LIB_DEPENDS
    RUN_DEPENDS
    TEST_DEPENDS

And lots of ports are going to make it FATAL now?

Regards, Sergei
Comment 6 Joe Marcus Clarke freebsd_committer 2020-05-05 21:14:49 UTC
Thanks for the feedback, Sergei.  Yeah, I agree re-ordering the dependencies is wrong.  Seems lots of Perl ports do this reverse declaration (which bugs me, but with the way make expands things, it works).  What about this diff (same URL as above, so refresh).

https://www.marcuscom.com/~jclarke/portlint.pl.diff
Comment 7 Sergei Vyshenski 2020-05-06 02:16:23 UTC
Marcus,

This time portlint becomes very tolerant to 
1) order of RUN_ and BUILD_ , and to 
2) "use then assign" for variables.

Point 2 is too common of practice in typical Makefile of port (consider include of bsd.port.mk in the last section of Makefile), and moreover - inside typical Mk/*.mk.

What do you think of demanding one of two reasonable approaches
(see 5.9.2. "RUN_DEPENDS" from Porter's Handbook),
each of which respects correct version of points 1) and 2) above:

(approach A) 
MY_DEPENDS=	...explicit list...
BUILD_DEPENDS=	${MY_DEPENDS}
RUN_DEPENDS=	${MY_DEPENDS}

(approach B)
BUILD_DEPENDS=	...explicit list for build...
RUN_DEPENDS=	...explicit list for run...

As for Perl ports which you mention, maybe they could be promoted to a new standards via automatic blanket commit?

Regards, Sergei
Comment 8 Joe Marcus Clarke freebsd_committer 2020-05-07 15:17:34 UTC
(In reply to Sergei Vyshenski from comment #7)

I like the MY_DEPENDS approach, but this would need portmgr buy-in to do a sweeping change.  That said, I don't see anything wrong per se in my patch.  The underlying checks are still there for other variables, and the explicit setting of RUN_DEPENDS to BUILD_DEPENDS (error) still holds.
Comment 9 Joe Marcus Clarke freebsd_committer 2020-11-29 17:20:25 UTC
Ports mgmt team, have a look at Comments 7 and 8 below.  Is this something that portmgr can do as a blanket commit?
Comment 10 Mathieu Arnold freebsd_committer 2020-11-29 17:32:35 UTC
Can you be a bit more explicit about what you are talking about?

The order in the Makefile is :

BUILD_DEPENDS= foo
RUN_DEPENDS=   bar

If you need the same build and run depends, you can write :

BUILD_DEPENDS= ${RUN_DEPENDS}
RUN_DEPENDS=   foo
Comment 11 Adam Weinberger freebsd_committer 2020-11-29 17:42:27 UTC
Use-before-define is perfectly valid in make syntax, so I'm not sure what we'd gain by shoehorning Makefiles to look more like valid syntax in other languages. I completely agree that it looks strange, but it's a valid pattern.

My take is that it's probably more incumbent upon portlint to recognize the valid syntactic pattern than it is for us to mandate what in some ways amounts to an anti-pattern.
Comment 12 Joe Marcus Clarke freebsd_committer 2020-11-29 17:59:22 UTC
In Comment 4, I propose a patch to do just that, Adam.  Sounds like I may have to go ahead with that.
Comment 13 Mathieu Arnold freebsd_committer 2020-11-30 09:58:58 UTC
I don't like the wording of your patch at all. It leaves a feeling that the user is doing something wrong, which they are not. I do not know how portlint evaluates the Makefile, but if, as it seems, it is trying to parse it line by line, it is really doing it wrong.  In any way, if a message is to be outputted at all, as saying something is probably wrong because the syntax is perfectly correct, it should be something like this:

> OK: $j refers to $1 (declared later), skipping checks.

make(1) does lazy evaluations of variables, it means that you can order them in whatever order you want, until they are used by a control structure (.if/.for/...), their content is not actually evaluated.
Comment 14 Joe Marcus Clarke freebsd_committer 2020-12-27 19:23:10 UTC
Thanks for the comments.  I have updated the wording to make this allowable and I no longer flag it as an error.