perf uses less at the default pager, but the linux-tools packages have no dependency for it

Bug #833101 reported by Dave Martin on 2011-08-24
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Linaro Linux
Won't Fix
Undecided
Unassigned
Linaro Ubuntu
Medium
Avik Sil
linux (Ubuntu)
Undecided
Brad Figg
linux-linaro (Ubuntu)
Undecided
Unassigned

Bug Description

$ perf list
sh: less: not found

This only affects minimal installs which don't have less installed.

Observed in:
 * linux-tools-2.6.38-11 (2.6.38-11.48) in the Ubuntu archive, running under natty (11.04)
 * linux-linaro-tools-3.0.0-1004 (3.0.0-1004.5~ppa~natty) from the linaro overlay PPA, from the 11.08 release.

The issue probably affects all packages containing the perf tool.

This could be fixed:

a) in linux (preferably upstream), by changing perf's default pager to more if less is absent
b) in debian/ubuntu, by changing perf's default pager to /usr/bin/pager or more
c) in debian/ubuntu by allowing util-linux to provide /usr/bin/less as an alternative using more.
d) in linux-meta by adding Depends: less (if believed to be acceptable)

This bug is missing log files that will aid in diagnosing the problem. From a terminal window please run:

apport-collect 833101

and then change the status of the bug to 'Confirmed'.

If, due to the nature of the issue you have encountered, you are unable to run this command, please add a comment stating that fact and change the bug status to 'Confirmed'.

This change has been made by an automated script, maintained by the Ubuntu Kernel Team.

Changed in linux (Ubuntu):
status: New → Incomplete
summary: perf uses less at the default pager, but the linux-tools packages have
- dependency for it
+ no dependency for it
Dave Martin (dave-martin-arm) wrote :

This issue only affects lightweight images which (coincidentally) don't have apport.

I believe this problem should be self-explanatory, even without additional logs.

Changed in linux (Ubuntu):
status: Incomplete → Confirmed
Dave Martin (dave-martin-arm) wrote :

Still present in linux-linaro-tools-3.0.0-1006

Changed in linaro-ubuntu:
milestone: none → 11.10
Fathi Boudra (fboudra) on 2011-11-11
Changed in linaro-ubuntu:
milestone: 11.10 → 11.11
Ricardo Salveti (rsalveti) wrote :

Avik, can you check if this is still an issue with our 11.11 images? Thanks.

Changed in linaro-ubuntu:
importance: Undecided → Medium
assignee: nobody → Avik Sil (aviksil)
Avik Sil (aviksil) wrote :

Yes, this issue is still present in 11.11 nano image.

Ricardo Salveti (rsalveti) wrote :

Thanks for confirming the issue. Will let the bug assigned to you then.

Changed in linaro-ubuntu:
status: New → Confirmed
milestone: 11.11 → 11.12
Ricardo Salveti (rsalveti) wrote :

Avik, any update on this one? If not yet fixed, please move it to 12.01. Thanks.

Avik Sil (aviksil) wrote :

No update as such. Moving it to 12.01.

Changed in linaro-ubuntu:
milestone: 11.12 → 12.01
Avik Sil (aviksil) wrote :

The attached patch seems to fix the issue.

tags: added: patch
Avik Sil (aviksil) wrote :

Patch not merged yet, moving it to 12.02

Changed in linaro-ubuntu:
milestone: 12.01 → 12.02
Avik Sil (aviksil) wrote :
Changed in linaro-ubuntu:
status: Confirmed → Fix Committed
no longer affects: linux-meta (Ubuntu)
Fathi Boudra (fboudra) on 2012-02-23
Changed in linaro-ubuntu:
status: Fix Committed → Fix Released
Brad Figg (brad-figg) wrote :

Granted the code in perf/util/pager.c looks borked in that "else if" after "if (!pager)" doesn't look like it will ever get executed. However, if you just set your PAGER env variable to either "more" or "cat" should do the job.

Changed in linux (Ubuntu):
assignee: nobody → Brad Figg (brad-figg)
status: Confirmed → In Progress
Avik Sil (aviksil) wrote :

Brad, does the patch in comment #11 look good here?

Brad Figg (brad-figg) wrote :

@Avik,

If it's as simple as setting the PAGER environment variable, why do I want to apply the patch?

If you think it's the right thing to do however, I'd suggest trying to push it upstream.

Dave Martin (dave-martin-arm) wrote :

PAGER is for customisation. A package which might not work if PAGER is not set is a broken package.

However, the proposed change (http://git.linaro.org/gitweb?p=people/ynk/linux-linaro-tracking.git;a=commit;h=49561d58a14351d136518f25bd686f9a6ca41b69) has some problems. In particular, it will mess up PATH in the environment, because strtok() causes the string it is used on to be modified. Since perf runs other subprocesses, this may lead to problems.

Why don't we just use /usr/bin/pager as the default? This is the whole point of the Debian alternatives system.

So:

If PAGER is set, try use its value. Don't bother to search: it's up to the user to specify sensible preferences.

Otherwise, if /usr/bin/pager exists, use that.

Otherwise, fall back to the legacy behaviour (i.e., try to run "less"). This should work just fine for the non-Debian folks.

Cheers
---Dave

Brad Figg (brad-figg) wrote :

@Dave,

I could live with something like that.

Avik Sil (aviksil) wrote :

Does this patch look OK?

diff --git a/tools/perf/util/pager.c b/tools/perf/util/pager.c
index 1915de2..d6f23de 100644
--- a/tools/perf/util/pager.c
+++ b/tools/perf/util/pager.c
@@ -57,8 +57,12 @@ void setup_pager(void)
        }
        if (!pager)
                pager = getenv("PAGER");
- if (!pager)
- pager = "less";
+ if (!pager) {
+ if (!access("/usr/bin/pager", X_OK))
+ pager = "/usr/bin/pager";
+ else
+ pager = "less";
+ }
        else if (!*pager || !strcmp(pager, "cat"))
                return;

On Thu, May 10, 2012 at 12:30:52PM -0000, Avik Sil wrote:
> Does this patch look OK?
>
> diff --git a/tools/perf/util/pager.c b/tools/perf/util/pager.c
> index 1915de2..d6f23de 100644
> --- a/tools/perf/util/pager.c
> +++ b/tools/perf/util/pager.c
> @@ -57,8 +57,12 @@ void setup_pager(void)
> }
> if (!pager)
> pager = getenv("PAGER");
> - if (!pager)
> - pager = "less";
> + if (!pager) {
> + if (!access("/usr/bin/pager", X_OK))
> + pager = "/usr/bin/pager";
> + else
> + pager = "less";

To conform to the existing code style, you might want to write the code
as follows:

+ if (!pager) {
+ if (!access("/usr/bin/pager", X_OK))
+ pager = "/usr/bin/pager";
+ }
+ if (!pager)
+ pager = "less";

Functionally, the code looks OK to me either way.

Cheers
---Dave

Linus Walleij (triad) wrote :

Packaging bug, won't fix in the kernel team.

Changed in linux-linaro:
status: New → Won't Fix
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers