Re: [PATCH] sysutils/screenfetch
Hi,
On Thu, 2 Aug 2018 18:07:33 -0400
Brian Callahan wrote:
> Hi Charlène --
>
> On 08/01/18 16:06, Charlène wrote:
> > Hi,
> >
> > I'm joining a new diff after Brian's comments.
> >
> > On Wed, 1 Aug 2018 02:17:37 -0400
> > Brian Callahan wrote:
> >
> >> Hi Charlène --
> >>
> >> On 07/30/18 14:22, Charlène wrote:
> >>> Hi,
> >>>
> >>> I'm proposing several changes to this port:
> >>>
> >>> Makefile:
> >>>
> >>> * cleaned extra tabs after '='
> >>> * use INSTALL_MAN instead of INSTALL_SCRIPT for the
> >>> manpage
> >>> * added myself as MAINTAINER, currently it's "orphaned"
> >>> * incremented REVISION
> >>> * added braces for variables
> >> Some of these I'm OK with, some I'm not super sure about.
> >> Yes, I think it's worthwhile to use INSTALL_MAN.
> >> Glad to see you taking MAINTAINER.
> >> Glad to see the updated patches.
> >>
> >> Braces around variables are not strictly needed in this case; it
> >> becomes a matter of style in this particular instance. But it might
> >> be even better to change PKGNAME to ${DISTNAME:L} and change
> >> GH_TAGNAME to v3.8.0 -- that would completely eliminate the need
> >> for a V variable in the first place.
> > I changed to what you recommend. You made me finally understand
> > that bsd.port.mk is a great source of documentation by itself,
> > thanks!
>
> Since it looks like we're going this route, Makefile.template says
> CATEGORIES comes after PKGNAME.
> So the first block should be:
> COMMENT
> PKGNAME
> REVISION
>
> Makefile.template also says that CATEGORIES comes after the GH_*
> variables, though I see plenty of ports that put CATEGORIES directly
> under DISTNAME/PKGNAME, as it would be if there were no GH_*
> variables. According to your diff, CATEGORIES was placed according to
> Makefile.template and be kept there.
>
> ~Brian
I reordered the Makefile according to Makefile.template.
Charlène.
> >> I'm more inclined to leave extra tabs in once a port has been
> >> imported, unless you're otherwise changing the line anyway, which
> >> you're not here. Especially in this case, where things are aligned
> >> anyway, I'm not sure removing the extra tabs improves readability.
> >> But I guess if you're going to do this, now would be the time.
> > I was advised to do so for sysutils/neofetch, so i applied the same
> > thing for this port. I entirely agree about the fact that it doesn't
> > improve (or worsen) readability here. I've let it as-is for this
> > time, but i take note for future works.
> >
> > Charlène.
> >
> >> ~Brian
> >>
> >>> patches, already in upstream's master branch but not in a release:
> >>>
> >>> * fixed "awk: cannot open /proc/fb" [1]
> >>> * fixed "unary operator expected" when no GPU is detected
> >>> [2]
> >>>
> >>> Testing:
> >>>
> >>> It has been successfully tested using proot, but you'll need to
> >>> copy /var/run/dmesg.boot in your chroot to check the output of the
> >>> program, as it uses this file for OS detection.
> >>>
> >>> Comments and OK are welcome!
> >>>
> >>> Charlène.
> >>>
> >>> [1]
> >>> https://github.com/KittyKatt/screenFetch/commit/dc72b5932e86ba9c4e36110408690abeb2004070
> >>> [2]
> >>> https://github.com/KittyKatt/screenFetch/commit/8346a75068323692243805fa702d02ec7538f3b9
> >>>
> >>>
>
screenfetch.diff
Description: Binary data
Re: [PATCH] sysutils/screenfetch
Hi Charlène --
On 08/01/18 16:06, Charlène wrote:
Hi,
I'm joining a new diff after Brian's comments.
On Wed, 1 Aug 2018 02:17:37 -0400
Brian Callahan wrote:
Hi Charlène --
On 07/30/18 14:22, Charlène wrote:
Hi,
I'm proposing several changes to this port:
Makefile:
* cleaned extra tabs after '='
* use INSTALL_MAN instead of INSTALL_SCRIPT for the manpage
* added myself as MAINTAINER, currently it's "orphaned"
* incremented REVISION
* added braces for variables
Some of these I'm OK with, some I'm not super sure about.
Yes, I think it's worthwhile to use INSTALL_MAN.
Glad to see you taking MAINTAINER.
Glad to see the updated patches.
Braces around variables are not strictly needed in this case; it
becomes a matter of style in this particular instance. But it might
be even better to change PKGNAME to ${DISTNAME:L} and change
GH_TAGNAME to v3.8.0 -- that would completely eliminate the need for
a V variable in the first place.
I changed to what you recommend. You made me finally understand
that bsd.port.mk is a great source of documentation by itself,
thanks!
Since it looks like we're going this route, Makefile.template says
CATEGORIES comes after PKGNAME.
So the first block should be:
COMMENT
PKGNAME
REVISION
Makefile.template also says that CATEGORIES comes after the GH_*
variables, though I see plenty of ports that put CATEGORIES directly
under DISTNAME/PKGNAME, as it would be if there were no GH_* variables.
According to your diff, CATEGORIES was placed according to
Makefile.template and be kept there.
~Brian
I'm more inclined to leave extra tabs in once a port has been
imported, unless you're otherwise changing the line anyway, which
you're not here. Especially in this case, where things are aligned
anyway, I'm not sure removing the extra tabs improves readability.
But I guess if you're going to do this, now would be the time.
I was advised to do so for sysutils/neofetch, so i applied the same
thing for this port. I entirely agree about the fact that it doesn't
improve (or worsen) readability here. I've let it as-is for this time,
but i take note for future works.
Charlène.
~Brian
patches, already in upstream's master branch but not in a release:
* fixed "awk: cannot open /proc/fb" [1]
* fixed "unary operator expected" when no GPU is detected
[2]
Testing:
It has been successfully tested using proot, but you'll need to
copy /var/run/dmesg.boot in your chroot to check the output of the
program, as it uses this file for OS detection.
Comments and OK are welcome!
Charlène.
[1]
https://github.com/KittyKatt/screenFetch/commit/dc72b5932e86ba9c4e36110408690abeb2004070
[2]
https://github.com/KittyKatt/screenFetch/commit/8346a75068323692243805fa702d02ec7538f3b9
Re: [PATCH] sysutils/screenfetch
Hi,
I'm joining a new diff after Brian's comments.
On Wed, 1 Aug 2018 02:17:37 -0400
Brian Callahan wrote:
> Hi Charlène --
>
> On 07/30/18 14:22, Charlène wrote:
> > Hi,
> >
> > I'm proposing several changes to this port:
> >
> > Makefile:
> >
> > * cleaned extra tabs after '='
> > * use INSTALL_MAN instead of INSTALL_SCRIPT for the manpage
> > * added myself as MAINTAINER, currently it's "orphaned"
> > * incremented REVISION
> > * added braces for variables
>
> Some of these I'm OK with, some I'm not super sure about.
> Yes, I think it's worthwhile to use INSTALL_MAN.
> Glad to see you taking MAINTAINER.
> Glad to see the updated patches.
>
> Braces around variables are not strictly needed in this case; it
> becomes a matter of style in this particular instance. But it might
> be even better to change PKGNAME to ${DISTNAME:L} and change
> GH_TAGNAME to v3.8.0 -- that would completely eliminate the need for
> a V variable in the first place.
I changed to what you recommend. You made me finally understand
that bsd.port.mk is a great source of documentation by itself,
thanks!
> I'm more inclined to leave extra tabs in once a port has been
> imported, unless you're otherwise changing the line anyway, which
> you're not here. Especially in this case, where things are aligned
> anyway, I'm not sure removing the extra tabs improves readability.
> But I guess if you're going to do this, now would be the time.
I was advised to do so for sysutils/neofetch, so i applied the same
thing for this port. I entirely agree about the fact that it doesn't
improve (or worsen) readability here. I've let it as-is for this time,
but i take note for future works.
Charlène.
> ~Brian
>
> > patches, already in upstream's master branch but not in a release:
> >
> > * fixed "awk: cannot open /proc/fb" [1]
> > * fixed "unary operator expected" when no GPU is detected
> > [2]
> >
> > Testing:
> >
> > It has been successfully tested using proot, but you'll need to
> > copy /var/run/dmesg.boot in your chroot to check the output of the
> > program, as it uses this file for OS detection.
> >
> > Comments and OK are welcome!
> >
> > Charlène.
> >
> > [1]
> > https://github.com/KittyKatt/screenFetch/commit/dc72b5932e86ba9c4e36110408690abeb2004070
> > [2]
> > https://github.com/KittyKatt/screenFetch/commit/8346a75068323692243805fa702d02ec7538f3b9
> >
> >
>
screenfetch.diff
Description: Binary data
Re: [PATCH] sysutils/screenfetch
Hi Charlène --
On 07/30/18 14:22, Charlène wrote:
Hi,
I'm proposing several changes to this port:
Makefile:
* cleaned extra tabs after '='
* use INSTALL_MAN instead of INSTALL_SCRIPT for the manpage
* added myself as MAINTAINER, currently it's "orphaned"
* incremented REVISION
* added braces for variables
Some of these I'm OK with, some I'm not super sure about.
Yes, I think it's worthwhile to use INSTALL_MAN.
Glad to see you taking MAINTAINER.
Glad to see the updated patches.
Braces around variables are not strictly needed in this case; it becomes
a matter of style in this particular instance. But it might be even
better to change PKGNAME to ${DISTNAME:L} and change GH_TAGNAME to
v3.8.0 -- that would completely eliminate the need for a V variable in
the first place.
I'm more inclined to leave extra tabs in once a port has been imported,
unless you're otherwise changing the line anyway, which you're not here.
Especially in this case, where things are aligned anyway, I'm not sure
removing the extra tabs improves readability. But I guess if you're
going to do this, now would be the time.
~Brian
patches, already in upstream's master branch but not in a release:
* fixed "awk: cannot open /proc/fb" [1]
* fixed "unary operator expected" when no GPU is detected [2]
Testing:
It has been successfully tested using proot, but you'll need to
copy /var/run/dmesg.boot in your chroot to check the output of the
program, as it uses this file for OS detection.
Comments and OK are welcome!
Charlène.
[1]
https://github.com/KittyKatt/screenFetch/commit/dc72b5932e86ba9c4e36110408690abeb2004070
[2]
https://github.com/KittyKatt/screenFetch/commit/8346a75068323692243805fa702d02ec7538f3b9
[PATCH] sysutils/screenfetch
Hi, I'm proposing several changes to this port: Makefile: * cleaned extra tabs after '=' * use INSTALL_MAN instead of INSTALL_SCRIPT for the manpage * added myself as MAINTAINER, currently it's "orphaned" * incremented REVISION * added braces for variables patches, already in upstream's master branch but not in a release: * fixed "awk: cannot open /proc/fb" [1] * fixed "unary operator expected" when no GPU is detected [2] Testing: It has been successfully tested using proot, but you'll need to copy /var/run/dmesg.boot in your chroot to check the output of the program, as it uses this file for OS detection. Comments and OK are welcome! Charlène. [1] https://github.com/KittyKatt/screenFetch/commit/dc72b5932e86ba9c4e36110408690abeb2004070 [2] https://github.com/KittyKatt/screenFetch/commit/8346a75068323692243805fa702d02ec7538f3b9 screenfetch.diff Description: Binary data
