From: Eli Zaretskii <eliz@gnu.org>
To: "Clément Pit--Claudel" <clement.pit@gmail.com>
Cc: emacs-devel@gnu.org
Subject: Re: Bisecting display bugs
Date: Mon, 11 Jul 2016 17:22:36 +0300 [thread overview]
Message-ID: <837fcsdzkj.fsf@gnu.org> (raw)
In-Reply-To: <5782E073.6000505@gmail.com> (message from Clément Pit--Claudel on Mon, 11 Jul 2016 01:55:31 +0200)
> From: Clément Pit--Claudel <clement.pit@gmail.com>
> Date: Mon, 11 Jul 2016 01:55:31 +0200
>
> I've written up notes while bisecting a display bug (#23938) on master. Since
> it didn't seem easy to identify the problem automatically from Elisp, I used a
> small `git bisect run' script that automatically captured, cropped, and
> compared screenshots of Emacs against a known-good reference. All in all,
> things worked nicely: git bisect took 4 hours to complete, but it didn't
> require any interaction after starting.
>
> I attached my notes. Is there interest in including them in admin/notes/?
Thank you for doing this. I'm okay with adding this kind of notes,
but I have some comments about the contents.
> +For general information about bisecting, use ‘M-x man git-bisect’.
^^^^^^^^^^^^^^^^^^
This doesn't work on Windows (the Windows port of Git doesn't install
man pages), suggest using "git help bisect" instead.
> +* Bisecting Emacs bugs
This section doesn't say anything "git help bisect" doesn't. It is
not Emacs-specific. I'm not sure we should repeat the text that is
quite clear in the Git documentation, and also includes useful
examples.
> +it's a new bug, a not-too-recent revision could do; otherwise,
> +checking for the bug previous releases of Emacs works well.
^^^
"in" missing here.
> +** Automating ‘git bisect’
> +
> +You can also write ‘git bisect run <some-shell-script>’ to automate
> +the process. ‘<some-shell-script>’ should build Emacs, run your bug
> +reproduction test, and return 0 if the current revision is good, and 1
> +otherwise. Returning 125 is equivalent to doing ‘git bisect skip’.
This part is also described on the Git bisect man page.
> +Concretely, ‘<some-shell-script>’ usually looks like this:
> +
> + #!/usr/bin/env bash
> +
> + # Remove leftovers from previous runs
> + git clean -xfd > /dev/null
> + # Build Emacs and skip commit if build fails
> + (./autogen.sh && ./configure --cache-file=/tmp/emacs.config.cache && make -j4) || exit 125
> +
> + # Reproduce the bug, writing output somewhere
> + src/emacs -Q -l "../reproduce-the-bug.el" || exit 125
> + # Remove leftovers again
> + git clean -xfd > /dev/null
> + # Analyze output and produce exit code
> + cmp ../reference-output current-output
IMO, this template could benefit from more generality. The result
doesn't have to come from 'cmp', it can come from 'diff' or some other
program, like 'cat' or 'ls'. It can also come from the exit code
produced by Emacs itself (which is probably a whole lot more
convenient when possible). Finally, what about bugs that cause Emacs
to crash? I think the template or the notes that come with it should
include some advice on that.
> +Some caveats:
> +
> +- This script cleans Emacs' source directory with ‘git clean -xfd’, so
> + make sure your uncommitted changes are saved somewhere else.
While bootstrapping after each step of bisect is safe, it makes the
run much longer, so I'd try bisecting without a bootstrap first,
especially if the range of commits to bisect is relatively small.
> +** Using ‘git bisect’ to find display-related bugs
> +
> +Most bugs that manifest graphically can be checked for by
> +programmatically inspecting text properties
Not just text properties: also the position of point, the text itself,
the window start position, and much more. I find the lack of details
in this part to be quite a gap, since familiarity with the respective
facilities is an important part of being efficient in debugging
display problems.
Specifically, I'd like to see at least the following facilities
mentioned here:
. window-start and window-end
. posn-at-point
. pos-visible-in-window-p
. frame-geometry
. window--dump-frame
. frame--size-history
. display-monitor-attributes-list
. C-u C-x =
. trace-redisplay and trace-to-stderr
. dump-glyph-matrix and dump-frame-glyph-matrix
Each one of these should be accompanied with a short (1-2 lines)
description of what it does and when is it useful.
> but some are only
> +apparent through visual inspection. Since building Emacs takes a long
> +time, it can be a pain to debug these manually.
I don't follow this logic: building is an automated process, while
visual inspection is usually very fast; and the automated comparison
you are about to suggest doesn't decrease the build time. So how does
the conclusion follow?
> +Fortunately, it's relatively easy to bisect such bugs automatically.
This is too optimistic, IMO. I would reword this in a more cautious
way, something like:
If the display bug has a clear manifestation in a screenshot of a
particular portion of Emacs display, and you have a program, like
'xwd', that can capture the content of the Emacs frame, and also
have ImageMagick installed, you can automate the comparison of the
redisplay results to make the bisection process fully automatic.
This technique, while valuable in certain situations, has quite a few
caveats that make it inappropriate in other situations, so we cannot
tell users "here's your magic wand". One important caveat is that the
size of an Emacs frame or window might not be constant across
revisions, so cropping the images correctly is not that trivial.
Another example of a caveat is when we deliberately change the visual
appearance of a part of the Emacs display (such as the mode line or
the tool bar) -- in these cases the baseline screenshot produced from
a "good" build will no longer be useful. And there are more caveats,
so being excessively optimistic when recommending this technique might
be a disservice to the reader.
> + (let ((window-id (frame-parameter nil 'outer-window-id)))
> + (call-process "xwd" nil nil nil "-silent" "-id" window-id "-out" fname))
> + (call-process "mogrify" nil nil nil fname "-crop" (format "%dx%d+%d+%d" w h x y))
> + (kill-emacs))
> +
> + (defun main ()
> + ;; Reproduce your bug here
> + …
> + ;; Force a redisplay
> + (redisplay t)
> + ;; Insert rough X, Y, W, H values below
> + (run-with-timer 0 nil #'take-screenshot-and-exit "screenshot.xwd" … … … …))
> +
> + (main)
> +
> +This script makes a screenshot of Emacs after reproducing your bug (if
> +‘xwd’ isn't available, you can use ImageMagick's ‘import’ tool,
> +passing it a ‘-window’ argument where ‘xwd’ wants ‘id’). Cropping the
> +image is useful to weed out unrelated display changes; try to include
> +only a small portion of the screen containing your bug.
> +
> +Then to produce the right exit code use ImageMagick's ‘identify’ tool:
> +
> + cmp <(identify -quiet -format "%#" "../screenshot.xwd") <(identify -quiet -format "%#" "../good.xwd")
If you're OK with invoking external programs from within Emacs, why
not compare the results using Emacs capabilities, and produce the
output via the exit code (kill-emacs can instruct Emacs to return an
exit code to the parent shell)? This has 2 advantages: (1) it doesn't
require the reader to be too proficient in Posix shell scripting, and
(2) the resulting script will be much more portable.
> --- a/admin/notes/repo
> +++ b/admin/notes/repo
> @@ -114,8 +114,7 @@ again.
> * Bisecting
>
> -This is a semi-automated way to find the revision that introduced a bug.
> -Browse 'git help bisect' for technical instructions.
> +See admin/notes/bisecting.
I wouldn't delete those 2 lines, they are useful for the reader to
decide whether she wants to read the details about bisecting.
Thanks again for working on this.
next prev parent reply other threads:[~2016-07-11 14:22 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-07-10 23:55 Bisecting display bugs Clément Pit--Claudel
2016-07-11 1:40 ` Óscar Fuentes
2016-07-11 14:22 ` Eli Zaretskii [this message]
2016-07-11 17:46 ` Óscar Fuentes
2016-07-11 17:54 ` Eli Zaretskii
2016-07-11 18:05 ` Óscar Fuentes
2016-07-22 18:55 ` Clément Pit--Claudel
2016-07-23 8:51 ` Eli Zaretskii
2016-07-23 13:22 ` Clément Pit--Claudel
2017-04-16 15:04 ` Clément Pit--Claudel
2017-04-16 15:32 ` Noam Postavsky
2017-04-16 20:57 ` Clément Pit-Claudel
2017-04-17 13:13 ` Herring, Davis
2017-04-17 13:43 ` Teemu Likonen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=837fcsdzkj.fsf@gnu.org \
--to=eliz@gnu.org \
--cc=clement.pit@gmail.com \
--cc=emacs-devel@gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.