Bug 193185
| Summary: | [stage] net/ipsorc MASTER_SITES LICENSE WWW take maintainership | ||
|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Chris Hutchinson <portmaster> |
| Component: | Individual Port(s) | Assignee: | John Marino <marino> |
| Status: | Closed FIXED | ||
| Severity: | Affects Many People | CC: | marino, portmaster |
| Priority: | --- | ||
| Version: | Latest | ||
| Hardware: | Any | ||
| OS: | Any | ||
| Attachments: | |||
I've corrected thia a number of times, but you haven't noticed: You aren't the maintainer yet, so you can't put "[maintainer]" in the title. It is misleading. Especially because this is a staging PR, please output: make check-plist make stage-qa portlint This PR has no obvious issues like the others, however it's also untested. So I won't reject the PR but I still insist on proof of testing. Created attachment 146764 [details]
net/ipsorc STAGE MASTER_SITES MAINTAINER LICENSE WWW pkg-decsr files/
net/ipsorc
Adds STAGEDIR, MASTER_SITES, LICENSE, WWW MAINTAINER
modifies pkg-descr, pkg-plist
renames files/patch-aa => files/patch-Makefile (to better conform to current naming scheme)
Please see 2014-09-03.diff, for details. See also net-ipsorc-tests-out
(also attached) for requisite QA testing log.
Thank you for all your time, and consideration.
--Chris
Created attachment 146765 [details]
net/ipsorc test log to accompany 2014-09-03.diff
net/ipsorc
Please find attached net-ipsorc-tests-out, the requisite QA testing
log to accompany 2014-09-03.diff (also attached).
Thank you for all your time, and consideration.
--Chris
The log looks good, but the new diff introduces a bunch of extra tables. Instead of the correct 2-tabbed columns, correct lines were moved to incorrect 3-tab alignment. I assume your editor wasn't set to 8 spaces per tab or something like thing. Please fix the tabs and then it's ready to go. Created attachment 146766 [details]
net/ipsorc OBSOLETES previous diff. Provides the same, but now makes Makefile ugly
net/ipsorc
OK here you are, John. As requested. :)
No. I use 4 space tabs. But hate it, when they aren't all
left justified on the same tab stop. Don't you?
But hey. I also can't stand it when the dollars in my wallet aren't
facing the same way, either. ;)
Thanks, John. I also appreciate you're demanding better QC on this
(and the others). As I needed to make some additional modifications,
and found a couple of additional pr(1)'s I get to send out to others
(those this port requires).
Best wishes.
--Chris
I still see two things. one minor: extract tab on MASTER_SITES line The real issue is the dirrm %%PORTDOCS%% line in pkg-plist. That should be removed list all the other PORTDOCS lines, right? spelling: extract => extra (In reply to C Hutchinson from comment #6) > Created attachment 146766 [details] > net/ipsorc OBSOLETES previous diff. Provides the same, but now makes > Makefile ugly > No. I use 4 space tabs. But hate it, when they aren't all > left justified on the same tab stop. Don't you? I don't understand. Use 8 spaces per tab for ports work. Then everything is aligned. It's only ugly for you. It looks correct for everyone else. Created attachment 146767 [details]
net/ipsorc OBSOLETES previous. JM is really pickey about tab stops -- grr...
net/ipsorc
Now it just feels like you're dogging me. But OK. Here you go.
I uploaded a modified copy of the previous.
Honestly, no sour grapes, mind you. But the previous, and most
all I can remember, where others were concerned, had the same number
of tabs on the additional MASTER_SITES line. I'm just trying to
find the pattern here. So I can keep with it. Not really complaining,
per se.
As to the pkg-plist %%PORTDOCS%%@dirrm %%DOCSDIR%% line.
No. I thought the same as you. But check-plist insisted
I use the method I have in there, currently. I double checked, and
all is added && removed, as expected/anticipated. So that's why it's
put in that way.
Thanks, John, and I hope that's really "it" this time. ;)
--Chris
(In reply to John Marino from comment #9) > (In reply to C Hutchinson from comment #6) > > Created attachment 146766 [details] > > net/ipsorc OBSOLETES previous diff. Provides the same, but now makes > > Makefile ugly > > > No. I use 4 space tabs. But hate it, when they aren't all > > left justified on the same tab stop. Don't you? > > I don't understand. > Use 8 spaces per tab for ports work. > Then everything is aligned. > It's only ugly for you. It looks correct for everyone else. Well that would explain it. I always use 4. But NP, I can live with it. Thanks for pointing it out. --Chris (In reply to C Hutchinson from comment #10) > Created attachment 146767 [details] > net/ipsorc OBSOLETES previous. JM is really pickey about tab stops -- grr... > > net/ipsorc > > Now it just feels like you're dogging me. But OK. Here you go. > I uploaded a modified copy of the previous. The tab wasn't the reason, I would have mentioned it but pushed it forward for the committer to fix (By the way, if you are relying on committers to fix little things, that's the wrong attitude. You should be shooting for zero corrections to your patches). > Honestly, no sour grapes, mind you. But the previous, and most > all I can remember, where others were concerned, had the same number > of tabs on the additional MASTER_SITES line. I'm just trying to > find the pattern here. So I can keep with it. Not really complaining, > per se. What are you talking about? I've corrected dozens of tabs for you before. You thought those were getting committed? If so, it means you aren't reviewing what gets committed versus what you submitted. (which I suspected because the same issues kept getting submitted) The standard tab number is two. Three can be used, but not on the PORTNAME, MAINTAINER, MASTER_SITE blocks. There it's always two. If your editor is set to anything other than 8 for tabs, then change it because it will cause you to commit badly tabbed lines. > As to the pkg-plist %%PORTDOCS%%@dirrm %%DOCSDIR%% line. > No. I thought the same as you. But check-plist insisted > I use the method I have in there, currently. I double checked, and > all is added && removed, as expected/anticipated. So that's why it's > put in that way. > > Thanks, John, and I hope that's really "it" this time. ;) Here we are again. You're just 100% sure about I guess. Never mind that it makes no sense to remove the files under %%PORTSDOCS%% but leave the directory? Do you see any other ports where only the directory is removed? This is check-plist misleading you. It's giving you the wrong advice. let's back up, why did you add this line: "DOCSDIR= ${PREFIX}/share/doc/${PORTNAME}" DOCSDIR is already defined. Why are you redefining it? and what's up with this line? "${INSTALL_DATA} ${PORTDOCS:S,^,${WRKSRC}/,} ${STAGEDIR}${DOCSDIR}" didn't we already establish that "(cd ${WRKSRC} && ${INSTALL_DATA} ${PORTSDOCS} ${STAGEDIR}${DOCSDIR})" is preferred? To be fair, what you have should work, but using regex unnecessary doesn't make it easier to maintain. You did the same on SCRIPTS. Did you see another port do that or it is something you invented? Something is wrong. It might be a bad check-plist logic caused by your redefinition of DOCSDIR, I'm not sure, but %%PORTSDOC%% definitely doesn't look right and it shouldn't be in pkg-plist at all. Adding it there could be masking a problem. (In reply to C Hutchinson from comment #11) > Well that would explain it. > I always use 4. But NP, I can live with it. Thanks for pointing it out. /me bands head against wall. If the issue was just affecting you, I wouldn't care. But we've seen that it affects your submissions, which means it affects everyone that deals with the PR it's submitted on. So I said, "Use tab=8 always, and you say you'll stick with 4 and deal". That tells me more mis-tabs are coming. And once you have a reputation for being sloppy, people are going to pass your PRs for those contributors that pay attention to detail. e.g. there are 10 PRs and you have time to pick up one. Are you going to pick up the PR from the guy with the great reputation, or the guy whose PRs always have issues? Even if they are only cosmetic issues? Think about it. (In reply to John Marino from comment #12) > (In reply to C Hutchinson from comment #10) > > Created attachment 146767 [details] > > net/ipsorc OBSOLETES previous. JM is really pickey about tab stops -- grr... > > > > net/ipsorc > > > > Now it just feels like you're dogging me. But OK. Here you go. > > I uploaded a modified copy of the previous. > > > The tab wasn't the reason, I would have mentioned it but pushed it forward > for the committer to fix (By the way, if you are relying on committers to > fix little things, that's the wrong attitude. You should be shooting for > zero corrections to your patches). No, No. I had no intention of pushing anything off. It was all meant to be tongue-in-cheek. It [single tab] was mentioned, seemed insignificant. So I joked about it. But, as mentioned further down. "I'm just trying to find the pattern here" {so I can stick with it] > > > > Honestly, no sour grapes, mind you. But the previous, and most > > all I can remember, where others were concerned, had the same number > > of tabs on the additional MASTER_SITES line. I'm just trying to > > find the pattern here. So I can keep with it. Not really complaining, > > per se. > > > What are you talking about? I've corrected dozens of tabs for you before. > You thought those were getting committed? If so, it means you aren't > reviewing what gets committed versus what you submitted. (which I suspected > because the same issues kept getting submitted) > > The standard tab number is two. Three can be used, but not on the PORTNAME, > MAINTAINER, MASTER_SITE blocks. There it's always two. If your editor is > set to anything other than 8 for tabs, then change it because it will cause > you to commit badly tabbed lines. No, no. See; no sour grapes && not complaining, per se. Just saying, not serious. Again, as also stated; "Just trying to find the pattern here". So I can get comfortable with it, and use it. > > > > > As to the pkg-plist %%PORTDOCS%%@dirrm %%DOCSDIR%% line. > > No. I thought the same as you. But check-plist insisted > > I use the method I have in there, currently. I double checked, and > > all is added && removed, as expected/anticipated. So that's why it's > > put in that way. > > > > Thanks, John, and I hope that's really "it" this time. ;) > > > Here we are again. You're just 100% sure about I guess. Never mind that it > makes no sense to remove the files under %%PORTSDOCS%% but leave the > directory? Do you see any other ports where only the directory is removed? > > This is check-plist misleading you. It's giving you the wrong advice. > > let's back up, why did you add this line: > "DOCSDIR= ${PREFIX}/share/doc/${PORTNAME}" > > DOCSDIR is already defined. Why are you redefining it? > > and what's up with this line? > "${INSTALL_DATA} ${PORTDOCS:S,^,${WRKSRC}/,} ${STAGEDIR}${DOCSDIR}" > > didn't we already establish that > "(cd ${WRKSRC} && ${INSTALL_DATA} ${PORTSDOCS} ${STAGEDIR}${DOCSDIR})" Right you are! Definitely, my bad. I took a short cut here, and _really_ should have known better. In short; I tried to make what was already there work. Rather than do it _correctly_ When check-plist complained, the impression I got, was that it didn't like the lines deleting the doc files. So I simply attempted to blow away the docs dir that was added, which ultimately clobbered the files, as well. It was clumsy, and I _really_ should have clobbered all the lines, and added correct ones. In the end the block would have been shorter, anyway. Prettier too. > > > is preferred? To be fair, what you have should work, but using regex > unnecessary doesn't make it easier to maintain. You did the same on > SCRIPTS. Did you see another port do that or it is something you invented? > > Something is wrong. It might be a bad check-plist logic caused by your > redefinition of DOCSDIR, I'm not sure, but %%PORTSDOC%% definitely doesn't > look right and it shouldn't be in pkg-plist at all. Adding it there could > be masking a problem. It came with the original. But it clobbers the added folder in share/docs. So I left it. If it bothers you. I'll re-create it from scratch. Thanks, John. --Chris (In reply to John Marino from comment #13) > (In reply to C Hutchinson from comment #11) > > Well that would explain it. > > I always use 4. But NP, I can live with it. Thanks for pointing it out. > > /me bands head against wall. > > If the issue was just affecting you, I wouldn't care. But we've seen that > it affects your submissions, which means it affects everyone that deals with > the PR it's submitted on. > > So I said, "Use tab=8 always, and you say you'll stick with 4 and deal". > That tells me more mis-tabs are coming. And once you have a reputation for > being sloppy, people are going to pass your PRs for those contributors that > pay attention to detail. e.g. there are 10 PRs and you have time to pick up > one. Are you going to pick up the PR from the guy with the great > reputation, or the guy whose PRs always have issues? Even if they are only > cosmetic issues? Think about it. Not true. Well, OK, true too. But. I can see the pattern(s) here. Especially now _knowing_ that most of the ports files are in an 8-space tab format. I can easily count, and you have also noted their [preferred] numbers. So long as I use the correct count, everyone will be happy. No? I have no intention, nor desire to be "sloppy". But I think everyone can win, in this particular case. No? Thanks, John. Points well taken. :) --Chris (In reply to C Hutchinson from comment #15) > I can see the pattern(s) here. Especially now _knowing_ that > most of the ports files are in an 8-space tab format. Not most, all. Anything that is exception needs to be changed. > I can easily count, and you have also noted their [preferred] numbers. > So long as I use the correct count, everyone will be happy. No? > > I have no intention, nor desire to be "sloppy". But I think > everyone can win, in this particular case. No? I truly don't believe that. You will make a mistake, because it's an easy mistake to make. I would also make a mistake if I tried it. I know, because I've tried this before. And you will deserve the criticism because you were strongly advised not to try it. > Just saying, not serious. I can't tell when you are joking, it's best to play this straight. I've been trying to help you contribute high quality submissions and I take what you say seriously. > Right you are! Definitely, my bad. > I took a short cut here, and _really_ should have known better. > In short; I tried to make what was already there work. Rather than do it > _correctly_ I want to be fair. What you have should work. At least, I don't see an immediate issue with it. This is a conformity / preference issue. > When check-plist complained, the impression I got, was that it didn't like > the lines deleting the doc files. So I simply attempted to blow away the > docs dir that was added, which ultimately clobbered the files, as well. > It was clumsy, and I _really_ should have clobbered all the lines, and > added correct ones. In the end the block would have been shorter, anyway. > Prettier too. I'm not sure I follow. When you define PORTDOCS, you get to remove all the lines starting with %%PORTDOCS%% in the pkg-plist. Not only get to, you have to, otherwise it gets added to the final list twice and fails to remove it twice during deinstallation > It came with the original. But it clobbers the added folder in share/docs. > So I left it. If it bothers you. I'll re-create it from scratch. remove DOCSDIR definition please. Let's see "make check-plist" after that. Created attachment 146776 [details]
net/ipsorc NEW PATCH OBSOLETES previous, addresses PORTDOCS, pkg-plist
net/ipsorc
OK. This I _think_ should meet all your "nits", and requirements.
I rewrote both related Makefile blocks, and redifined associated
pkg-plist entries.
Please find 2014-0-03.diff, and net-ipsorc-tests-output attached.
Please let me know if I've missed anything.
--Chris
Created attachment 146777 [details]
net/ipsorc : net-ipsorc-tests-output : QA test log
net/ipsorc
QA test log to accompany 2014-09-03.diff (net-ipsorc-test-output)
--Chris
(In reply to John Marino from comment #16) > (In reply to C Hutchinson from comment #15) > > I can see the pattern(s) here. Especially now _knowing_ that > > most of the ports files are in an 8-space tab format. > > > Not most, all. Anything that is exception needs to be changed. > > > > I can easily count, and you have also noted their [preferred] numbers. > > So long as I use the correct count, everyone will be happy. No? > > > > I have no intention, nor desire to be "sloppy". But I think > > everyone can win, in this particular case. No? > > > I truly don't believe that. You will make a mistake, because it's an easy > mistake to make. I would also make a mistake if I tried it. I know, > because I've tried this before. And you will deserve the criticism because > you were strongly advised not to try it. > > > > Just saying, not serious. > > > I can't tell when you are joking, it's best to play this straight. I've > been trying to help you contribute high quality submissions and I take what > you say seriously. > > > > Right you are! Definitely, my bad. > > I took a short cut here, and _really_ should have known better. > > In short; I tried to make what was already there work. Rather than do it > > _correctly_ > > > I want to be fair. What you have should work. At least, I don't see an > immediate issue with it. This is a conformity / preference issue. > > > > When check-plist complained, the impression I got, was that it didn't like > > the lines deleting the doc files. So I simply attempted to blow away the > > docs dir that was added, which ultimately clobbered the files, as well. > > It was clumsy, and I _really_ should have clobbered all the lines, and > > added correct ones. In the end the block would have been shorter, anyway. > > Prettier too. > > I'm not sure I follow. When you define PORTDOCS, you get to remove all the > lines starting with %%PORTDOCS%% in the pkg-plist. Not only get to, you > have to, otherwise it gets added to the final list twice and fails to remove > it twice during deinstallation > > > It came with the original. But it clobbers the added folder in share/docs. > > So I left it. If it bothers you. I'll re-create it from scratch. > > remove DOCSDIR definition please. > > Let's see "make check-plist" after that. Just for the record, and _in spite_ of anything I [might] say, to the contrary. I _do_ appreciate your critique. I'm under pressure, sometimes to get things done, or it's late, and I'm tired (it's 3:30am now). I _may_ seem contrary. But I _do_ [eventually] catch wind of my [better] senses. So _know_, I _do_ appreciate it. Thanks, John. I hope you find my latest changes to all this. More to your liking. OH, and don't get rid of that stick, you keep whacking me with, just yet. I may yet need a couple more. I'm stubborn [like you haven't noticed] :) --Chris Okay, I can't say too much because this works although I would have preferred that you keep using PORTDOCS. But that's a preference and not wrong. I will point out that you mis-tabbed "DOCS= README HOWTO". That is only tabbed once and you need two. So given that you made an error on your very first attempt, I hope that you believe that keeping ts=4 is foolish and more mistakes are coming. I'm moving this to patch-ready. It's good enough. One more thing, and I know I've told you this multiple times so hopefully this is the last time: You cannot have the first line, "# Created by: C Hutchinson <portmaster@bsdforge.com>" You did not create this port. The port already exists. You are re-writing history. It's like saying Jon Bon Jovi discovered America. If you can fix the tab and remove this line, please do. Otherwise the committer that takes this PR should do it. Created attachment 146819 [details]
net/ipsorc NEW PATCH OBSOLETES previous, addresses PORTDOCS, pkg-plist FINAL
net/ipsorc
Here it is. In all it's glory.
Tab added
Top line removed
I'll keep this one close by. For use as a formatting template.
Thanks, John.
--Chris
The pkg-plist is absurdly short, I'm removing this file. What's going with with "post-patch" target? This is an ugly "roll-your-own" when REINPLACE_CMD works just fine. how old is this port? The WRKSRC= definition is exactly the same as the default, it's useless. Removing. A commit references this bug: Author: marino Date: Sun Sep 7 10:49:27 UTC 2014 New revision: 367509 URL: http://svnweb.freebsd.org/changeset/ports/367509 Log: Stage net/ipsorc and assign maintainership to submitter PR: 193185 Submitted by: Chris Hutchinson Improvements: marino Changes: head/net/ipsorc/Makefile head/net/ipsorc/pkg-descr head/net/ipsorc/pkg-plist As indicated in the comments section today, I made several additional improvements to this port. It's committed now. (In reply to John Marino from comment #27) > As indicated in the comments section today, I made several additional > improvements to this port. It's committed now. Hello, John, and thank you for all your attention to this submission. One question; Would the following COMMENT have still met your requirment(s)? Tool to create and send IP packets with a GTK front-end Point being; it seems more indicative of it's use case, what with having/being a GUI, as opposed to CLI. Thanks again. --Chris |
Created attachment 146580 [details] net/ipsorc [maintainer] STAGE MASTER_SITES LICENSE WWW net/ipsorc adds MAINTAINER, MASTER_SITES, LICENSE, WWW removes DEAD MASTER_SITES link, and DEAD WWW link please see svn(1) diff(1), attached. --Chris