Added license information.
Use DISTVERSION instead of PORTVERSION.
Pet portfmt.
Details
Details
- Reviewers
bofh diizzy - Commits
- R11:6837e7316b15: sysutils/clean: Add UNKNOWN License
Portlint, portfmt, portclippy passed.
Poudriere testport fine on amd64 13.2, 14.0.
Warning about files/patch-Makefile not generating using "make makepatch".
Diff Detail
Diff Detail
- Repository
- R11 FreeBSD ports repository
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
Reiterating the informations about patching:
- Whenever updating a software try to refresh the patches. That is make patch and make makepatch.
- If you have any post-patch target on the same files make sure to disable/uncomment the post-patch target otherwise you will have improper patches.
- makepatch format is better for builders. Patching is highly CPU intensive so even if the diffs/hunks are skewed by 1 line it creates extra CPU cycles in the builders.
- If the patch is an upstream commit utilize the PATCH_SITES and PATCHFILES compared to local patches.
- Although you will see me personally utilizing lots of REINPLACE_CMD this is not intended except for smaller cases which are really clear with the search and replace like what it is doing. For example when you are rewriting /usr with LOCALBASE, or replacing CC feel free to utilize REINPLACE_CMD.
Hope this helps.
Comment Actions
This is a bit awkward that you are running sed twice on the same file. These should be really avoided because this is what happens in the background:
- The first time it is run there is a new Makefile.bak
- The second time it is run that Makefile.bak is overwritten with the Makefile altered at first stage
- So the original Makefile is lost
So how about:
${REINPLACE_CMD} -e 's| .(PREFIX)| $$(DESTDIR)$$(PREFIX)|g' ${WRKSRC}/MakefileThese are tidbits we try to follow and not mandatory for this review:
- For the sake on sanity please have a blank line in between last line(.include <bsd.port.mk>) and the previous line.
- Prefer | or # rather than ,. Comma is a confusing character in terms of the location of the keyboard and often are the cause of errors due to typos.
Comment Actions
Yes, you've right.
So how about:
${REINPLACE_CMD} -e 's| .(PREFIX)| $$(DESTDIR)$$(PREFIX)|g' ${WRKSRC}/Makefile
It isn't right because the manual page's directory will be PREFIX/man/man1 instead of PREFIX/share/man/man1 (the share subdirectory missed).
What do you think about 's|$$(PREFIX)|$$(DESTDIR)$$(PREFIX)| ; s|man/man1|share/man/man1|' ?
These are tidbits we try to follow and not mandatory for this review:
- For the sake on sanity please have a blank line in between last line(.include <bsd.port.mk>) and the previous line.
Ah, yes, I see. It's strange none of portlinters find this mistake.
- Prefer | or # rather than ,. Comma is a confusing character in terms of the location of the keyboard and often are the cause of errors due to typos.
Okay. I choose the comma because is a small character and easily see the boundaries.
Comment Actions
Yeap you are correct, I missed the man share/man thing. For the last couple of days I have worked too much on the share/man that they look the same to me. :D
These are tidbits we try to follow and not mandatory for this review:
- For the sake on sanity please have a blank line in between last line(.include <bsd.port.mk>) and the previous line.
Ah, yes, I see. It's strange none of portlinters find this mistake.
yes none of the linters actually report this hence you need to add an extra eye. :D
And to let you know that none of the linters are actually correct.
- Prefer | or # rather than ,. Comma is a confusing character in terms of the location of the keyboard and often are the cause of errors due to typos.
Okay. I choose the comma because is a small character and easily see the boundaries.
It's a personal preference. You are free to chose on your own ports but on others ports it is preferable to use | or #. I tend to avoid it as I have couple of times misplaced , with . by typos and couldn't figure it out.
Comment Actions
Fix REINPLACE_CMD.
Planned commit message:
sysutils/clean: take maintainership
Added license information.
Use DISTVERSION instead of PORTVERSION.
Use REINPLACE_CMD instead of patching.
Pet portfmt.
Comment Actions
Although the patch looks fine to me the planned commit message is not.
This is not the most important part of the entire commit log or what you have done here and hence should not be going on the heading. From the entire work it should be sysutils/clean: Add UNKNOWN License.
And the preferred term is Adopt port. This is actually the least important of the work and of no significance to others except for errors. So this should go in the body.
Remember noone is the owner of a specific port they are just the shepherd of the port. :D
Added license information.
While adding or changing LICENSE mention the LICENSE name. The is Add UNKNOWN License. While this is a license not from the bsd.license.db.mk try to add the License Text and where you have found it. License is a very crucial part and also very much confusing and making any sort of mistakes is not really acceptable. Specially when there is no LICENSE or COPYING file in the distfile. So often we have to make judgement based on availability. To clear yourself from any judicial issues hence it's best to add the source of the information like, this is the license text based or mentioned in the file main.c or whatever the file is.
Use DISTVERSION instead of PORTVERSION.
Prefer DISTVERSION ..
Use REINPLACE_CMD instead of patching.
Preferred lingo is Refresh patches utilizing REINPLACE_CMD
Pet portfmt.
Comment Actions
Thanks for fixes. Will be good the commit message below?
sysutils/clean: Add UNKNOWN License
The license text can found in main.c.
Prefer DISTVERSION.
Refresh patches utilizing REINPLACE_CMD.
Adopt port.
Approved by: bofh
Differential revision: https://reviews.freebsd.org/D44179