Bug 148109 - [patch] audio/ladspa: avoid custom make targets
Summary: [patch] audio/ladspa: avoid custom make targets
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: Normal Affects Only Me
Assignee: Tilman Keskinoz
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-24 11:20 UTC by swell.k
Modified: 2010-07-04 16:30 UTC (History)
0 users

See Also:


Attachments
a.diff (1.59 KB, patch)
2010-06-24 11:20 UTC, swell.k
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description swell.k 2010-06-24 11:20:07 UTC
- use build/install logic from distfile, the port depends
  on devel/gmake, anyway
- respect CC/CXX from environ and make.conf
- make post-patch target quiet, not install target

How-To-Repeat: $ env CC=clang CXX=clang++ make build
Comment 1 Edwin Groothuis freebsd_committer 2010-06-24 11:25:32 UTC
Responsible Changed
From-To: freebsd-ports-bugs->arved

Over to maintainer (via the GNATS Auto Assign Tool)
Comment 2 dfilter service freebsd_committer 2010-07-02 19:50:30 UTC
arved       2010-07-02 18:50:21 UTC

  FreeBSD ports repository

  Modified files:
    audio/ladspa         Makefile 
  Log:
  Respect CC CXX.
  
  PR:             148109
  Submitted by:   Anonymous <swell.k@gmail.com>
  Feature safe:   yes
  
  Revision  Changes    Path
  1.15      +2 -2      ports/audio/ladspa/Makefile
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
Comment 3 Tilman Keskinoz freebsd_committer 2010-07-02 19:50:53 UTC
State Changed
From-To: open->closed

Thanks for reporting. The port should now buld with clang
Comment 4 swell.k 2010-07-03 03:46:30 UTC
arved@FreeBSD.org writes:

> Synopsis: [patch] audio/ladspa: avoid custom make targets
>
> State-Changed-From-To: open->closed
> State-Changed-By: arved
> State-Changed-When: Fri Jul 2 18:50:53 UTC 2010
> State-Changed-Why: 
> Thanks for reporting. The port should now buld with clang

You're aware that was a change-request, not a sw-bug? Unless you give me
a reason for rejecting the rest of the patch, I'm requesting that you
reopen the PR.

For example, I still think ports should be verbose about installing
files, strip(1)ing plugins on install, use _MAKE_JOBS when
FORCE_MAKE_JOBS is set and respect CXXFLAGS from environment or
make.conf. Avoiding custom do-* targets reduces the chance of such bugs.

BTW, you can mark the port as MAKE_JOBS_SAFE, it successfully builds
with -j10, not with your custom do-build target, though.

>
> http://www.freebsd.org/cgi/query-pr.cgi?pr=148109
Comment 5 swell.k 2010-07-03 03:46:30 UTC
arved@FreeBSD.org writes:

> Synopsis: [patch] audio/ladspa: avoid custom make targets
>
> State-Changed-From-To: open->closed
> State-Changed-By: arved
> State-Changed-When: Fri Jul 2 18:50:53 UTC 2010
> State-Changed-Why: 
> Thanks for reporting. The port should now buld with clang

You're aware that was a change-request, not a sw-bug? Unless you give me
a reason for rejecting the rest of the patch, I'm requesting that you
reopen the PR.

For example, I still think ports should be verbose about installing
files, strip(1)ing plugins on install, use _MAKE_JOBS when
FORCE_MAKE_JOBS is set and respect CXXFLAGS from environment or
make.conf. Avoiding custom do-* targets reduces the chance of such bugs.

BTW, you can mark the port as MAKE_JOBS_SAFE, it successfully builds
with -j10, not with your custom do-build target, though.

>
> http://www.freebsd.org/cgi/query-pr.cgi?pr=148109
Comment 6 dfilter service freebsd_committer 2010-07-03 15:15:58 UTC
arved       2010-07-03 14:15:40 UTC

  FreeBSD ports repository

  Modified files:
    audio/ladspa         Makefile 
  Log:
  Respect CXXFLAGS
  Make port profit from MAKE_JOBS
  
  PR:             148109
  Requested by:   Anonymous <swell.k@gmail.com>
  Feature safe:   yes
  
  Revision  Changes    Path
  1.16      +6 -4      ports/audio/ladspa/Makefile
_______________________________________________
cvs-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "cvs-all-unsubscribe@freebsd.org"
Comment 7 Tilman Keskinoz freebsd_committer 2010-07-03 15:18:16 UTC
* Anonymous [2010-07-03 13:54]:
> You're aware that was a change-request, not a sw-bug? Unless you give me
> a reason for rejecting the rest of the patch, I'm requesting that you
> reopen the PR.
> 
> For example, I still think ports should be verbose about installing
> files, strip(1)ing plugins on install, use _MAKE_JOBS when
> FORCE_MAKE_JOBS is set and respect CXXFLAGS from environment or
> make.conf. Avoiding custom do-* targets reduces the chance of such bugs.
> 
> BTW, you can mark the port as MAKE_JOBS_SAFE, it successfully builds
> with -j10, not with your custom do-build target, though.

Ok, i made the port respect CXXFLAGS and MAKE_JOBS_SAFE. Thanks.

I did not strip the plugins, because then i would have to bump PORTREVISION, which is not
worth it imho. If i remember i will do it with the next update.
Comment 8 swell.k 2010-07-03 16:59:56 UTC
Tilman Linneweh <arved@FreeBSD.org> writes:

> * Anonymous [2010-07-03 13:54]:
>> You're aware that was a change-request, not a sw-bug? Unless you give me
>> a reason for rejecting the rest of the patch, I'm requesting that you
>> reopen the PR.
>> 
>> For example, I still think ports should be verbose about installing
>> files, strip(1)ing plugins on install, use _MAKE_JOBS when
>> FORCE_MAKE_JOBS is set and respect CXXFLAGS from environment or
>> make.conf. Avoiding custom do-* targets reduces the chance of such bugs.
>> 
>> BTW, you can mark the port as MAKE_JOBS_SAFE, it successfully builds
>> with -j10, not with your custom do-build target, though.
>
> Ok, i made the port respect CXXFLAGS and MAKE_JOBS_SAFE. Thanks.

Good.

>
> I did not strip the plugins, because then i would have to bump PORTREVISION, which is not
> worth it imho. If i remember i will do it with the next update.

Please do and remove custom do-install target as well. It's better to
trust vendor makefile for available plugins.

However, changing CC/CXX and CXXFLAGS already requires PORTREVISION bump
because it *affects* build. And you're abandoning the bug about
respecting STRIP... Considering how often ladspa_sdk releases I guess
the PR would be suspended for a few years.

BTW, I think having one simple patch is better than making those regexps
even more ugly. That file wasn't generated by automake, anyway.

  $ cd $(make -V BUILD_WRKSRC)
  $ gmake install PREFIX=$HOME/aaa
  cc -I. -Wall -Werror  -fPIC -o plugins/amp.o -c plugins/amp.c
  ld -o ../plugins/amp.so plugins/amp.o -shared
  cc -I. -Wall -Werror  -fPIC -o plugins/delay.o -c plugins/delay.c
  ld -o ../plugins/delay.so plugins/delay.o -shared
  cc -I. -Wall -Werror  -fPIC -o plugins/filter.o -c plugins/filter.c
  ld -o ../plugins/filter.so plugins/filter.o -shared
  cc -I. -Wall -Werror  -fPIC -o plugins/noise.o -c plugins/noise.c
  ld -o ../plugins/noise.so plugins/noise.o -shared
  g++ -I. -Wall -Werror  -fPIC -o plugins/sine.o -c plugins/sine.cpp
  g++ -o ../plugins/sine.so plugins/sine.o -shared
  cc -I. -Wall -Werror  -fPIC   -c -o analyseplugin.o analyseplugin.c
  cc -I. -Wall -Werror  -fPIC   -c -o load.o load.c
  cc -I. -Wall -Werror  -fPIC   -c -o default.o default.c
  cc -I. -Wall -Werror  -fPIC -lm                                 \
                  -o ../bin/analyseplugin                                 \
                  analyseplugin.o load.o default.o
  cc -I. -Wall -Werror  -fPIC   -c -o applyplugin.o applyplugin.c
  cc -I. -Wall -Werror  -fPIC -lm                                 \
                  -o ../bin/applyplugin                                   \
                  applyplugin.o load.o default.o
  cc -I. -Wall -Werror  -fPIC   -c -o listplugins.o listplugins.c
  cc -I. -Wall -Werror  -fPIC   -c -o search.o search.c
  cc -I. -Wall -Werror  -fPIC -lm                                 \
                  -o ../bin/listplugins                                   \
                  listplugins.o search.o
  install -d /home/holo/aaa/lib/ladspa
  install -d /home/holo/aaa/include
  install -d /home/holo/aaa/bin
  install -s -m 555 ../plugins/* /home/holo/aaa/lib/ladspa
  install -m 444 ladspa.h /home/holo/aaa/include
  install -s -m 555 ../bin/* /home/holo/aaa/bin

--- a.diff begins here ---
Index: audio/ladspa/Makefile
===================================================================
RCS file: /a/.cvsup/ports/audio/ladspa/Makefile,v
retrieving revision 1.16
diff -u -p -r1.16 Makefile
--- audio/ladspa/Makefile	3 Jul 2010 14:15:17 -0000	1.16
+++ audio/ladspa/Makefile	3 Jul 2010 15:38:00 -0000
@@ -20,27 +20,11 @@ LICENSE_FILE=	${WRKSRC}/doc/COPYING
 WRKSRC=		${WRKDIR}/ladspa_sdk
 USE_GMAKE=	yes
 USE_LDCONFIG=	yes
-PROGRAM_FILES=	analyseplugin applyplugin listplugins
-PLUGIN_FILES=	amp.so delay.so filter.so noise.so sine.so
-PLUGIN_DIR?=	${PREFIX}/lib/ladspa/
+MAKE_ENV+=	INSTALL_PROGRAM="${INSTALL_PROGRAM}" INSTALL_DATA="${INSTALL_DATA}"
 MAKEFILE=	makefile
 ALL_TARGET=	targets
 BUILD_WRKSRC=	${WRKSRC}/src
+INSTALL_WRKSRC=	${BUILD_WRKSRC}
 MAKE_JOBS_SAFE=	yes
 
-post-patch:
-	${REINPLACE_CMD} -e 's,-ldl,,; s,-O3,${CFLAGS},; \
-		s,^CC,#CC,;s,^CPP,#CPP,;s,CPP,CXX,; s,^CXXFLAGS.*=,CXXFLAGS+=,' \
-		${WRKSRC}/src/makefile
-
-do-install:
-	@${INSTALL_DATA} ${WRKSRC}/src/ladspa.h ${PREFIX}/include
-.for file in ${PROGRAM_FILES}
-	@${INSTALL_PROGRAM} ${WRKSRC}/bin/${file} ${PREFIX}/bin
-.endfor
-	@${MKDIR} ${PLUGIN_DIR}
-.for file in ${PLUGIN_FILES}
-	@${INSTALL_DATA} ${WRKSRC}/plugins/${file} ${PLUGIN_DIR}
-.endfor
-
 .include <bsd.port.mk>
Index: audio/ladspa/files/patch-makefile
===================================================================
RCS file: audio/ladspa/files/patch-makefile
diff -N audio/ladspa/files/patch-makefile
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ audio/ladspa/files/patch-makefile	3 Jul 2010 15:25:56 -0000
@@ -0,0 +1,71 @@
+--- src/makefile.orig	2007-11-06 13:42:45.000000000 +0300
++++ src/makefile	2010-07-03 19:22:33.576284227 +0400
+@@ -4,9 +4,7 @@
+ #
+ # Change these if you want to install somewhere else.
+ 
+-INSTALL_PLUGINS_DIR	=	/usr/lib/ladspa/
+-INSTALL_INCLUDE_DIR	=	/usr/include/
+-INSTALL_BINARY_DIR	=	/usr/bin/
++PREFIX		?=	/usr
+ 
+ ###############################################################################
+ #
+@@ -14,9 +12,9 @@ INSTALL_BINARY_DIR	=	/usr/bin/
+ #
+ 
+ INCLUDES	=	-I.
+-LIBRARIES	=	-ldl -lm
+-CFLAGS		=	$(INCLUDES) -Wall -Werror -O3 -fPIC
+-CXXFLAGS	=	$(CFLAGS)
++LIBRARIES	=	-lm
++CFLAGS		+=	$(INCLUDES) -Wall -Werror  -fPIC
++CXXFLAGS	+=	$(CFLAGS)
+ PLUGINS		=	../plugins/amp.so				\
+ 			../plugins/delay.so				\
+ 			../plugins/filter.so				\
+@@ -25,8 +23,12 @@ PLUGINS		=	../plugins/amp.so				\
+ PROGRAMS	=	../bin/analyseplugin				\
+ 			../bin/applyplugin 				\
+ 			../bin/listplugins
+-CC		=	cc
+-CPP		=	c++
++CC		?=	cc
++CXX		?=	c++
++INSTALL_DIR	?=	install -d
++INSTALL_PROGRAM	?=	install $(STRIP) -m 555
++INSTALL_DATA	?=	install -m 444
++STRIP		?=	-s
+ 
+ ###############################################################################
+ #
+@@ -38,8 +40,8 @@ CPP		=	c++
+ 	$(LD) -o ../plugins/$*.so plugins/$*.o -shared
+ 
+ ../plugins/%.so:	plugins/%.cpp ladspa.h
+-	$(CPP) $(CXXFLAGS) -o plugins/$*.o -c plugins/$*.cpp
+-	$(CPP) -o ../plugins/$*.so plugins/$*.o -shared
++	$(CXX) $(CXXFLAGS) -o plugins/$*.o -c plugins/$*.cpp
++	$(CXX) -o ../plugins/$*.so plugins/$*.o -shared
+ 
+ ###############################################################################
+ #
+@@ -59,12 +61,12 @@ test:	/tmp/test.wav ../snd/noise.wav alw
+ 	@echo Test complete.
+ 
+ install:	targets
+-	-mkdirhier $(INSTALL_PLUGINS_DIR)
+-	-mkdirhier $(INSTALL_INCLUDE_DIR)
+-	-mkdirhier $(INSTALL_BINARY_DIR)
+-	cp ../plugins/* $(INSTALL_PLUGINS_DIR)
+-	cp ladspa.h $(INSTALL_INCLUDE_DIR)
+-	cp ../bin/* $(INSTALL_BINARY_DIR)
++	$(INSTALL_DIR) $(PREFIX)/lib/ladspa
++	$(INSTALL_DIR) $(PREFIX)/include
++	$(INSTALL_DIR) $(PREFIX)/bin
++	$(INSTALL_PROGRAM) ../plugins/* $(PREFIX)/lib/ladspa
++	$(INSTALL_DATA) ladspa.h $(PREFIX)/include
++	$(INSTALL_PROGRAM) ../bin/* $(PREFIX)/bin
+ 
+ /tmp/test.wav:	targets ../snd/noise.wav
+ 	../bin/listplugins
--- a.diff ends here ---
Comment 9 Tilman Keskinoz freebsd_committer 2010-07-03 19:51:18 UTC
Hello Anonymous,

On Jul 3, 2010, at 17:59 , Anonymous wrote:
>=20
> Please do and remove custom do-install target as well. It's better to
> trust vendor makefile for available plugins.

Please provide reason, why it is better to make a 70lines patch instead =
of
a 8 lines do-install target.=20

Generally the FreeBSD port maintainer knows better where files should be =
installed on FreeBSD, than the Linux-centric upstream author.

>=20
> However, changing CC/CXX and CXXFLAGS already requires PORTREVISION =
bump
> because it *affects* build. And you're abandoning the bug about
> respecting STRIP... Considering how often ladspa_sdk releases I guess
> the PR would be suspended for a few years.

No, for the supported default environment the package did not change.=
Comment 10 swell.k 2010-07-04 16:21:04 UTC
arved@FreeBSD.org writes:

> Hello Anonymous,
>
> On Jul 3, 2010, at 17:59 , Anonymous wrote:
>>
>> Please do and remove custom do-install target as well. It's better to
>> trust vendor makefile for available plugins.
>
> Please provide reason, why it is better to make a 70lines patch instead of
> a 8 lines do-install target.

Well, if ladspa development wasn't frozen then I'd try to submit it
upstream. But I need to test it on Linux first.

If you care so much about number of lines why not use post-patch target
from my first diff? It takes only 3 lines of regexps but your custom
target uses 8 lines.

+               -e 's,/usr,${PREFIX},; s,-mkdirhier,${INSTALL} -d,;' \
+		-e '/install:/,/^$$/{ /(BINARY|PLUGINS)_DIR/s,cp,${INSTALL_PROGRAM},; \
+			/INCLUDE_DIR/s,cp,${INSTALL_DATA},; }' \

And you can reduce 10 lines if you dispose of FILESDIR/patch-applyplugin.c
to one-liner.

>
> Generally the FreeBSD port maintainer knows better where files should
> be installed on FreeBSD, than the Linux-centric upstream author.

It does not mean patches should be FreeBSD-specific, too.

>
>> 
>> However, changing CC/CXX and CXXFLAGS already requires PORTREVISION bump
>> because it *affects* build. And you're abandoning the bug about
>> respecting STRIP... Considering how often ladspa_sdk releases I guess
>> the PR would be suspended for a few years.
>
> No, for the supported default environment the package did not change.

So, respecting STRIP (bugfix) is less important than PORTREVISION?