Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net-analyzer/nload: EAPI=7, fix bug 663402 "speed graph rendering issue" #9543

Closed
wants to merge 4 commits into from

Conversation

ahippo
Copy link
Contributor

@ahippo ahippo commented Aug 12, 2018

The first patch is a preparation, which adds -r1 ebuild, and fixes repoman warnings for it.
(switches to EAPI=7 and shortens the package description).
The second patch adds an upstream patch to fix the issue described in Gentoo bug 663402.

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Aug 12, 2018
@ahippo
Copy link
Contributor Author

ahippo commented Aug 12, 2018

build.log is attached to bug 663402.


inherit autotools

DESCRIPTION="Real time network traffic and bandwidth usage monitor for the text console"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bad translation and suggest
DESCRIPTION="Real time network traffic monitor for the command line interface"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I'll fix the description and update the PR.

ahippo added a commit to ahippo/gentoo that referenced this pull request Aug 14, 2018
Add -r1 ebuild to fix the following repoman warnings:
"""
  DESCRIPTION.toolong           1
   net-analyzer/nload/nload-0.7.4.ebuild: DESCRIPTION is 83 characters (max 80)
  repo.eapi-deprecated          1
   net-analyzer/nload/nload-0.7.4.ebuild: 5
"""

The description string is suggested by @jonasstein.

Package-Manager: Portage-2.3.40, Repoman-2.3.9

Link: gentoo#9543 (comment)
@ahippo
Copy link
Contributor Author

ahippo commented Aug 14, 2018

@jonasstein, I've updated the description per your suggestion.
Please, let me know if you have any other comments.

ahippo added a commit to ahippo/gentoo that referenced this pull request Aug 21, 2018
Add -r1 ebuild to fix the following repoman warnings:
"""
  DESCRIPTION.toolong           1
   net-analyzer/nload/nload-0.7.4.ebuild: DESCRIPTION is 83 characters (max 80)
  repo.eapi-deprecated          1
   net-analyzer/nload/nload-0.7.4.ebuild: 5
"""

The description string is suggested by @jonasstein.

Package-Manager: Portage-2.3.40, Repoman-2.3.9

Link: gentoo#9543 (comment)
@ahippo
Copy link
Contributor Author

ahippo commented Aug 21, 2018

Looks like, @gentoo/netmon could use some help.
I've added myself as a proxied maintainer.
(unless @gentoo/proxy-maint has any objections)

If there is not much use of me being a proxied maintainer, simply discard the last commit before merging.

@ahippo ahippo changed the title net-analyzer/nload: EAPI=7, fix bug 663402 "speed graph rendering issue" net-analyzer/nload: EAPI=7, fix bug 663402 "speed graph rendering issue" [please reassign] Aug 21, 2018
@gentoo-bot gentoo-bot changed the title net-analyzer/nload: EAPI=7, fix bug 663402 "speed graph rendering issue" [please reassign] net-analyzer/nload: EAPI=7, fix bug 663402 "speed graph rendering issue" Aug 21, 2018
@gentoo-bot
Copy link

Pull Request assignment

Areas affected: ebuilds
Packages affected: net-analyzer/nload

net-analyzer/nload: @gentoo/netmon

Bugs linked: 663402

In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. and removed assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Aug 21, 2018
@ahippo
Copy link
Contributor Author

ahippo commented Aug 23, 2018

@mgorny, would Proxy Maint team be able to help me with merging this PR?
Sorry to call you out specifically, but looks like, I can't mention the whole @gentoo/proxy-maint team.

HOMEPAGE="http://www.roland-riegel.de/nload/index.html"
SRC_URI="http://www.roland-riegel.de/nload/${P}.tar.gz"

LICENSE="GPL-2"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LICENSE is wrong:

 * This program is free software; you can redistribute it and/or modify
 * it under the terms of the GNU General Public License as published by
 * the Free Software Foundation; either version 2 of the License, or
 * (at your option) any later version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, thank you!
I'll fix and update the PR.

Add -r1 ebuild to fix the following repoman warnings:
"""
  DESCRIPTION.toolong           1
   net-analyzer/nload/nload-0.7.4.ebuild: DESCRIPTION is 83 characters (max 80)
  repo.eapi-deprecated          1
   net-analyzer/nload/nload-0.7.4.ebuild: 5
"""

The description string is suggested by @jonasstein.

Correct the package license in the ebuild as pointed out by @mgorny.
The source code says:
"""
/***************************************************************************
 *                                                                         *
 *   This program is free software; you can redistribute it and/or modify  *
 *   it under the terms of the GNU General Public License as published by  *
 *   the Free Software Foundation; either version 2 of the License, or     *
 *   (at your option) any later version.                                   *
 *                                                                         *
 ***************************************************************************/
"""

Link: gentoo#9543 (comment)
Link: gentoo#9543 (comment)
Package-Manager: Portage-2.3.40, Repoman-2.3.9
Apply the following patch from upstream:
8a93886 "Eliminate flicker on some terminals like rxvt (thanks to Alex Wilson)"

This fixes network utilization graph rendering issue.
(the text on the right side of the screen
slides to the left together with graph updates)

Closes: https://bugs.gentoo.org/663402
Closes: gentoo#9543
Package-Manager: Portage-2.3.40, Repoman-2.3.9
@ahippo
Copy link
Contributor Author

ahippo commented Aug 24, 2018

I've fixed the license in commit a863959 (I've amended the fix).
Also, I reordered the commits, so that the last one is the one to close the PR.

@mgorny
Copy link
Member

mgorny commented Aug 24, 2018

>>> /usr/share/man/man1/nload.1.gz.lz

Something is compressing installed manpages. Please make sure they are installed uncompressed so that Portage can compress them.

@ahippo
Copy link
Contributor Author

ahippo commented Aug 24, 2018

/usr/share/man/man1/nload.1.gz.lz
Something is compressing installed manpages. Please make sure they are installed uncompressed so that Portage can compress them.

Oh, that's weird.
Thank you for pointing this out!
I didn't notice this somehow.

@ahippo
Copy link
Contributor Author

ahippo commented Aug 25, 2018

/usr/share/man/man1/nload.1.gz.lz
Something is compressing installed manpages. Please make sure they are installed uncompressed so that Portage can compress them.

Oh, that's weird.
Thank you for pointing this out!
I didn't notice this somehow.

I didn't notice that because on my machine it installs the normal nload.1.bz2 manpage:

gzip -f /var/tmp/portage/net-analyzer/nload-0.7.4-r1/image/usr/share/man/man1/nload.1
...
>>> /usr/share/man/man1/nload.1.bz2

make install does compress the manpage with gzip, but ecompressdir handles this fine.
(I'm running sys-apps/portage-2.3.40-r1)

ecompressdir has a special code to run the proper decompressor saying "# not uncommon for packages to compress doc files themselves".
So, I'm not sure if the ebuild needs to handle this specially somehow.

docs/Makefile.am might be doing it wrong indeed -- it doesn't just use man1_MANS, but also defines an explicit install target for the manpage.
However, the change appears to be deliberate for some reason.

@mgorny
Copy link
Member

mgorny commented Aug 25, 2018

You need to handle it. Portage implementation does not follow the standards, and I've deliberately changed it to catch this.

@ahippo
Copy link
Contributor Author

ahippo commented Aug 25, 2018

You need to handle it. Portage implementation does not follow the standards, and I've deliberately changed it to catch this.

OK, let me fix the Makefile.am then.
By the way, what standard mandates this? PMS?

@mgorny
Copy link
Member

mgorny commented Aug 25, 2018

Yes. It doesn't specify that files must be decompressed from other formats before compression.

ahippo added a commit to ahippo/nload that referenced this pull request Aug 26, 2018
Package managers like to compress man-pages on their own,
because the type of compression for man-pages is user-configurable.
In particular, Gentoo [1] doesn't want packages to compress their man-pages.
Gentoo Portage has workarounds for this,
but this is not specified in Package Manager Specification and
results in extra compression-decompression pass.

RPM also compresses man-pages itself (in `brp-compress`)
(and similarly recompresses them as needed)
rather than relying on packages to install compressed man-pages.

Automake can handle installation of man-pages without the explicit "install" target,
so use the standard automake-provided way of installing man-pages.
It's also smart enough to package `nload.1.in` automatically.

Don't specify an explicit man-page extension in .spec file as recommended by Fedora.

[1] gentoo/gentoo#9543 (comment)
As pointed out by @mgorny [1], packages should not compress their man pages on their own.
Instead, Portage needs to handle the compression.
Currently, `ecompressdir` is smart enough to recompress the man-pages as needed.
However, this is not a fully PMS-compliant behavior.

So, patch the Makefile.am not to compress the man-page.
A corresponding pull request is submitted upstream [2].

[1] gentoo#9543 (comment)
[2] rolandriegel/nload#4
@ahippo
Copy link
Contributor Author

ahippo commented Aug 26, 2018

Man-page compression should have been fixed in 68dc8ff

@ahippo
Copy link
Contributor Author

ahippo commented Aug 26, 2018

Yes. It doesn't specify that files must be decompressed from other formats before compression.

I see.
Thanks for the info!

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2018-08-26 03:18 UTC
Newest commit scanned: 68dc8ff
Status: ✅ good

No issues found

@mgorny
Copy link
Member

mgorny commented Aug 26, 2018

Thank you!

gentoo-bot pushed a commit that referenced this pull request Aug 26, 2018
gentoo-bot pushed a commit that referenced this pull request Aug 26, 2018
Add -r1 ebuild to fix the following repoman warnings:
"""
  DESCRIPTION.toolong           1
   net-analyzer/nload/nload-0.7.4.ebuild: DESCRIPTION is 83 characters (max 80)
  repo.eapi-deprecated          1
   net-analyzer/nload/nload-0.7.4.ebuild: 5
"""

The description string is suggested by @jonasstein.

Correct the package license in the ebuild as pointed out by @mgorny.
The source code says:
"""
/***************************************************************************
 *                                                                         *
 *   This program is free software; you can redistribute it and/or modify  *
 *   it under the terms of the GNU General Public License as published by  *
 *   the Free Software Foundation; either version 2 of the License, or     *
 *   (at your option) any later version.                                   *
 *                                                                         *
 ***************************************************************************/
"""

Link: #9543 (comment)
Link: #9543 (comment)
Package-Manager: Portage-2.3.40, Repoman-2.3.9
gentoo-bot pushed a commit that referenced this pull request Aug 26, 2018
As pointed out by @mgorny [1], packages should not compress their man pages on their own.
Instead, Portage needs to handle the compression.
Currently, `ecompressdir` is smart enough to recompress the man-pages as needed.
However, this is not a fully PMS-compliant behavior.

So, patch the Makefile.am not to compress the man-page.
A corresponding pull request is submitted upstream [2].

[1] #9543 (comment)
[2] rolandriegel/nload#4

Closes: #9543
@ahippo ahippo deleted the net-analyzer/nload branch August 26, 2018 17:13
@ahippo
Copy link
Contributor Author

ahippo commented Aug 26, 2018

Thank you for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR.
Projects
None yet
5 participants