Page MenuHomeFreeBSD

graphics/pfstools: [PATCH]fix qt option build
ClosedPublic

Authored by ultima on Jul 5 2017, 8:03 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Dec 14, 4:16 PM
Unknown Object (File)
Sat, Dec 14, 4:13 PM
Unknown Object (File)
Thu, Dec 5, 12:49 PM
Unknown Object (File)
Thu, Dec 5, 11:37 AM
Unknown Object (File)
Tue, Nov 26, 8:20 PM
Unknown Object (File)
Nov 23 2024, 2:52 AM
Unknown Object (File)
Nov 7 2024, 3:04 PM
Unknown Object (File)
Sep 18 2024, 4:43 PM
Subscribers

Details

Summary
  • Fix qt option build

PR\: 220247
Sumitted by\: Iouri V. Ivliev <fbsd@any.com.ru> (maintainer)
Reported by\: Jov <amutu@amutu.com>
Reviewed by\: lifanov (mentor), matthew (mentor)
Approved by\: lifanov (mentor), matthew (mentor)
Differential Revision\: https://reviews.freebsd.org/DXXXXX

Test Plan

portlint:
WARN: Makefile: [68]: use of != in assignments is almost never a good thing to do. Try to avoid using them. See http://lists.freebsd.org/pipermail/freebsd-ports/2008-July/049777.html for some helpful hints on what to do instead.
WARN: Makefile: for new port, make $FreeBSD$ tag in comment section empty, to make SVN happy.
WARN: Makefile: new ports should not set PORTREVISION.
0 fatal errors and 3 warnings found.

Not sure what the correct solution for #1 is. This use case of varible assignment is foreign to me.
Leaving it alone, if it isn't broke don't fix it?

After reading more about !=, it looks like it just means the variable is executed before being assigned. Still not sure if it would work better in a target.

poudriere:
103i386
103amd64
110i386
110amd64
12i386
12amd64

Diff Detail

Repository
rP FreeBSD ports repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

The != assignments are like a performance hand-grenade that splatters everyone around it.
The right-hand side expression would get evaluated every time the file is processed....
In this case I see that it's inside of a non-default option and it would be ugly to work around it,
so it might not be so bad. I'll let other people chime in.

This chunk:

.if ${PORT_OPTIONS:MOCTAVE}
OCTAVE_BASE?=   ${LOCALBASE}
OCTAVE_VERSION!=${OCTAVE_BASE}/bin/octave-config -v 2>&1 || ${ECHO} "0"
OCTAVE_SITE_OCT!=${OCTAVE_BASE}/bin/octave-config --oct-site-dir 2>&1 || ${ECHO} ""
OCTAVE_SITE_M!= ${OCTAVE_BASE}/bin/octave-config --m-site-dir 2>&1 || ${ECHO} ""
PLIST_SUB+=     \
                OCTAVE_BASE=${OCTAVE_BASE} \
                OCTAVE_SITE_M=${OCTAVE_SITE_M:S|^${OCTAVE_BASE}/||} \
                OCTAVE_SITE_OCT=${OCTAVE_SITE_OCT:S|^${OCTAVE_BASE}/||}
USES+=          shebangfix
SHEBANG_FILES=  src/octave/*
SHEBANG_LANG+=  octave
octave_OLD_CMD?=/usr/bin/octave
octave_CMD?=    ${OCTAVE_BASE}/bin/octave
.endif

would be a real performance drag if the OCTAVE option was on by default. You can factor out all of the SHEBANG related stuff
by using options helpers there, but the PLIST_SUB related parts are tricky. I don't think you can set a make(1) variable as
the action of a target. Hmmm....

mat requested changes to this revision.Jul 6 2017, 7:42 AM
mat added inline comments.
graphics/pfstools/Makefile
82 ↗(On Diff #30459)

post-install. Never use post-stage unless you need to do something later that will not work during post-install.

This revision now requires changes to proceed.Jul 6 2017, 7:42 AM
graphics/pfstools/Makefile
82 ↗(On Diff #30459)

I'm not sure why the maintainer wants to change this bit, I'll do some tests and make a query.

ultima edited edge metadata.
  • Added octave uses option helper, reverted the post-stage to post-install

This chunk:

.if ${PORT_OPTIONS:MOCTAVE}
OCTAVE_BASE?=   ${LOCALBASE}
OCTAVE_VERSION!=${OCTAVE_BASE}/bin/octave-config -v 2>&1 || ${ECHO} "0"
OCTAVE_SITE_OCT!=${OCTAVE_BASE}/bin/octave-config --oct-site-dir 2>&1 || ${ECHO} ""
OCTAVE_SITE_M!= ${OCTAVE_BASE}/bin/octave-config --m-site-dir 2>&1 || ${ECHO} ""
PLIST_SUB+=     \
                OCTAVE_BASE=${OCTAVE_BASE} \
                OCTAVE_SITE_M=${OCTAVE_SITE_M:S|^${OCTAVE_BASE}/||} \
                OCTAVE_SITE_OCT=${OCTAVE_SITE_OCT:S|^${OCTAVE_BASE}/||}
USES+=          shebangfix
SHEBANG_FILES=  src/octave/*
SHEBANG_LANG+=  octave
octave_OLD_CMD?=/usr/bin/octave
octave_CMD?=    ${OCTAVE_BASE}/bin/octave
.endif

would be a real performance drag if the OCTAVE option was on by default. You can factor out all of the SHEBANG related stuff
by using options helpers there, but the PLIST_SUB related parts are tricky. I don't think you can set a make(1) variable as
the action of a target. Hmmm....

I tired moving the other SHEBANG vars, but it shebangfix doesn't recognize them, so only changed the USES.

I still need commit approval for this, the shebandfix doesn't seem to work with option helpers, so only added the USES option helper.

This revision was automatically updated to reflect the committed changes.
head/graphics/pfstools/Makefile
77–78

Both those are not needed.

head/graphics/pfstools/Makefile
77–78

Can you explain this one further please? I am looking through Mk/* and can't find these entries anywhere. Are you positive they are not needed?

head/graphics/pfstools/Makefile
77–78

because you add octave to SHEBANG_LANG. For all entries in SHEBANG_LANG, <e>_OLD_CMD and <e>_CMD are automatically generated. https://www.freebsd.org/doc/en/books/porters-handbook/uses-shebangfix.html

head/graphics/pfstools/Makefile
77–78

Didn't realize the *_CMD variables were automatically generated, I thought they were required if not using one of the languages listed in Uses/shebangfix.mk.

I need approval for:

Index: Makefile

  • Makefile (revision 445722)

+++ Makefile (working copy)
@@ -75,8 +75,6 @@

OCTAVE_SITE_OCT=${OCTAVE_SITE_OCT:S|^${OCTAVE_BASE}/||}

SHEBANG_FILES= src/octave/*
SHEBANG_LANG+= octave
-octave_OLD_CMD?=/usr/bin/octave
-octave_CMD?= ${OCTAVE_BASE}/bin/octave
.endif

post-install:

matthew accepted this revision.

Approved

This revision was automatically updated to reflect the committed changes.