Update to 0.8.3 release. Tested: portlint, tinderbox build, pkg_add / pkg_delete, runtime Thanks! Fix: Patch attached with submission follows:
Responsible Changed From-To: freebsd-ports-bugs->jgh I'll take it.
jgh 2012-01-24 21:51:31 UTC FreeBSD ports repository Modified files: devel/dulwich Makefile distinfo pkg-plist Log: - Update to 0.8.3 PR: ports/164417 Approved by: rene (mentor) Revision Changes Path 1.13 +4 -2 ports/devel/dulwich/Makefile 1.12 +2 -2 ports/devel/dulwich/distinfo 1.8 +6 -0 ports/devel/dulwich/pkg-plist _______________________________________________ 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"
State Changed From-To: open->closed Committed, with minor changes. Thanks!
On Tuesday 24 January 2012 22:51:54 you wrote: > Synopsis: [maintainer-update] [patch] devel/dulwich: Update to 0.8.3 > > State-Changed-From-To: open->closed > State-Changed-By: jgh > State-Changed-When: Tue Jan 24 21:51:54 UTC 2012 > State-Changed-Why: > Committed, with minor changes. Thanks! > > http://www.freebsd.org/cgi/query-pr.cgi?pr=164417 Hello, thank you for taking the PR and committing! May I ask you to revert these 'minor changes', please? Reasons below. Patch attached. First, the change was completely unnecessary. Second, the loop is neither necessary nor efficient and just a waste of resources in itself compared to the previous version. Third, the 'cd ${WRKSRC}' is also in the loop and is executed for every doc in PORTDOCS, so instead of just one directory change there are now five of them. That is both wrong (bug) and a waste of resources. Forth, the loop turns a simple and efficient oneliner into three lines, which is unnecessary, explicitly unwanted and inefficient. For the reasons above and the general goal to keep the Makefiles simple, efficient and small, the change should be reverted to the previous version, which is much better in every aspect and was explicitly introduced for good reasons. Thank you very much for your understanding! Regards
PORTSDOC is only a list of documents, however it doesn't mean that proper usage should not be taken into account for installation. Since this is a list of files, it should not be used in cooperation with the "install" binary in the manner it was used previously. Please consider the attached patch, as a compromise. -jgh -- Jason Helfman | FreeBSD Committer jgh@FreeBSD.org | http://people.freebsd.org/~jgh
On Wednesday 25 January 2012 18:12:06 Jason Helfman wrote: > PORTSDOC is only a list of documents, however it doesn't mean that proper > usage should not be taken into account for installation. Since this is a > list of files, it should not be used in cooperation with the "install" > binary in the manner it was used previously. Could you please explain what was wrong / why it should not be used that way? I would like to understand it. The install(1) manual page states as synopsis: install [options] file1 ... fileN directory And since ${PORTDOCS} will be substituted with its content (files) before execution, it is just the same as above synopsis line. I do not know what is wrong with it. Did I missed something? BTW, sunpoet@ introduced the oneliner in revision 1.7 more than a year ago. Since then none of the other committers complained about that line. It is funny that one committer introduced it and another one reverts it. There should be some sort of agreement. Otherwise it is an uncomfortable situation for the maintainer. > Please consider the attached patch, as a compromise. If you actually insist on the for loop, then yes, please take that instead. Thank you for your passion and time. Regards
I'll jump in at this point, and point out that it was me who told Jason that he should be using for loops in this way. Sorry for the confusion here-- you're correct that it's a valid usage in most cases. The danger comes when people use something like: PORTDOCS= bluurgh bluurgh2 ${INSTALL_DATA} ${WRKSRC}/${PORTDOCS} ${DOCSDIR} which expands to ${INSTALL_DATA} ${WRKSRC}/bluurgh bluurgh2 ${DOCSDIR} I'm sure you can see the problem here. The cd in this case is a nice alternative method around that, and I'm sorry that that was missed-- please blame me and not Jason in this case. Normally changes like this aren't worth the CVS noise, but I understand that the principle of your requests as maintainer override this; we could revert it now or on the next update. I'll leave the choice up to you. Chris
> Sorry for the confusion here-- you're correct that it's a valid usage > in most cases. The danger comes when people use something like: > > PORTDOCS= bluurgh bluurgh2 > > ${INSTALL_DATA} ${WRKSRC}/${PORTDOCS} ${DOCSDIR} > > which expands to > > ${INSTALL_DATA} ${WRKSRC}/bluurgh bluurgh2 ${DOCSDIR} > > I'm sure you can see the problem here. Ah, that is it. Thanks for your explanation! > The cd in this case is a nice alternative method around that, and I'm sorry > that that was missed-- please blame me and not Jason in this case. It is really not my intention to blame anyone. I know that you (all committers) do a pretty good job and I appreciate that very much. I just wanted to understand it, which I do now. > Normally changes like this aren't worth the CVS noise, Agreed, but ... > but I understand that the principle of your requests as maintainer override > this; we could revert it now or on the next update. I'll leave the > choice up to you. It would be nice if it could be done now. So could you please do it? I believe upstream will not release the next update anytime soon. Many thanks! Regards