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

Bug #833101 reported by Dave Martin
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Linaro Linux
Won't Fix
Undecided
Unassigned
Linaro Ubuntu
Fix Released
Medium
Avik Sil
linux (Ubuntu)
In Progress
Undecided
Brad Figg
linux-linaro (Ubuntu)
New
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)

Tags: patch
Revision history for this message
Brad Figg (brad-figg) wrote : Missing required logs.

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
Revision history for this message
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
Revision history for this message
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)
Changed in linaro-ubuntu:
milestone: 11.10 → 11.11
Revision history for this message
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)
Revision history for this message
Avik Sil (aviksil) wrote :

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

Revision history for this message
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
Revision history for this message
Ricardo Salveti (rsalveti) wrote :

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

Revision history for this message
Avik Sil (aviksil) wrote :

No update as such. Moving it to 12.01.

Changed in linaro-ubuntu:
milestone: 11.12 → 12.01
Revision history for this message
Avik Sil (aviksil) wrote :

The attached patch seems to fix the issue.

tags: added: patch
Revision history for this message
Avik Sil (aviksil) wrote :

Patch not merged yet, moving it to 12.02

Changed in linaro-ubuntu:
milestone: 12.01 → 12.02
Revision history for this message
Avik Sil (aviksil) wrote :
Changed in linaro-ubuntu:
status: Confirmed → Fix Committed
no longer affects: linux-meta (Ubuntu)
Fathi Boudra (fboudra)
Changed in linaro-ubuntu:
status: Fix Committed → Fix Released
Revision history for this message
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
Revision history for this message
Avik Sil (aviksil) wrote :

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

Revision history for this message
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.

Revision history for this message
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

Revision history for this message
Brad Figg (brad-figg) wrote :

@Dave,

I could live with something like that.

Revision history for this message
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;

Revision history for this message
Dave Martin (dave-martin-arm) wrote : Re: [Bug 833101] Re: perf uses less at the default pager, but the linux-tools packages have no dependency for it

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

Revision history for this message
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  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.