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

Add helper script for bisecting regressions #3214

Merged
merged 1 commit into from
Jan 28, 2019

Conversation

fingolfin
Copy link
Member

This script makes it quite convenient to track down regression as in issue #3212 or #3205.

@fingolfin fingolfin added the release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes label Jan 22, 2019
@codecov
Copy link

codecov bot commented Jan 22, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@9abb317). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3214   +/-   ##
=========================================
  Coverage          ?   84.76%           
=========================================
  Files             ?      687           
  Lines             ?   336051           
  Branches          ?        0           
=========================================
  Hits              ?   284856           
  Misses            ?    51195           
  Partials          ?        0

@coveralls
Copy link

coveralls commented Jan 22, 2019

Coverage Status

Coverage increased (+0.009%) to 84.928% when pulling 4ca5e6b on fingolfin:mh/bisect into 9abb317 on gap-system:master.

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

This is a very useful idea.
When trying to test the script, I found the following problems.

  • I entered git bisect bad master, and git told me that I have to start with git bisect start.
  • I entered git bisect good stable-4.9, and git told me fatal: Needed a single version
    and Bad rev input: stable-4.9.
    Perhaps git expects that I have fetched the stable-4.9 branch before?
  • I fetched the stable-4.9 branch, entered git bisect good stable-4.9 again,
    and git told me Bisecting: a merge base must be tested.
    This is irritating if one is not aware that it might happen.
  • I entered git bisect run ..., got a lot of output lines, and then messages
    that etc/bisect.sh was not found, and bisect run failed.
    Apparently etc/bisect.sh is not available in that situation because a version is checked out
    where this file does not yet exist.

@fingolfin
Copy link
Member Author

@ThomasBreuer Thanks for the feedback. I will add git bisect start to the instructions; I didn't add it because git offers to do it for me (and AFAICT, it always does that)

$ git bisect bad master
You need to start by "git bisect start"
Do you want me to do it for you [Y/n]?

And yes, all refs (such as master or stable-4.9) that one passes to git bisect must exist, just like with any other git command.

Regarding the "a mege base must be tested": I'll add a comment that git bisect will checkout all kinds of different revisions, and that one should read the git bisect man page before doing any of this.

Finally, on the error about etc/bisect.sh not being found: oops, that's of course a crucial point; I didn't consider that when I used it before. I guess I'll have to add instructions to first make a copy of the script somewhere.

@fingolfin
Copy link
Member Author

I tried to improve the text, please take another look and let me know what you think!

There is another issue that should be addressed, though: there is a range of ~200 commits (from f3da6b3 to d2ea52e) in which GAP does not start for me, because it already requires transpkg, but is not compatible with the current version of transpkg: I get this error when starting GAP:

...
Error, BIND_GLOBAL: variable `TRANSREGION' must be unbound at ./lib/global.g:137 called from
BIND_GLOBAL( name, value ); at ./lib/global.gi:259 called from
<function "BindGlobal">( <arguments> )
 called from read-eval loop at ./pkg/transgrp/lib/trans.gd:79
you can 'quit;' to quit to outer loop, or
you can 'return;' to continue
brk>

I need to come up with a way to workaround this: either by somehow convincing GAP to start anyway (e.g. apply a temporary fix to lib/read4.g which removes the offending lines -- but that seems fragile); or else by finding a way to detect this brokenness (unfortunately, the error happens before the custom OnQuit function is set), and then instruct git to skip the commit (but then git will just switch to the "next" commit... and since there are ~200 commits in a row that are broken, that is highly undesirable).

Another idea for making GAP start with option -A for the commits in the named ranges: Perhaps the script can set up another fake GAPROOT dir in some tmp dir, then copy read4.g there, patch it, and instruct GAP to use that GAP dir as first root dir... Hrm, sounds rather complicated...

Any other (better) ideas?

@fingolfin
Copy link
Member Author

OK, I think I came up with a working (although still ugly) solution for the TRANSREGION problem. I've updated the PR, but I'll need to test this some more.

@fingolfin
Copy link
Member Author

Seems to work fine now. As a concrete example, this is how one can reproduce my manual bisection of issue #3205: go to the GAP repository root dir, add a file literals.tst with this content:

gap> SizeScreen([80,100]);;
gap> Display(f->212345678901234567890123456789012345678901234567890123456789012345678901234567890);
function ( f )
    return 
     2123456789012345678901234567890123456789012345678901234567890123456789012\
34567890;
end

Then, initiate the bisection (you may need to run git fetch --tags first to get the bisect-after-new-buildsys tag):

cp etc/bisect.sh .
git bisect start
git bisect bad master
git bisect good bisect-after-new-buildsys
git bisect run ./bisect.sh literals.tst

This runs for a while, and eventually ends this way:

...
c78bb4c4d89a8c17bb931675df6d39a87b9e32d3 is the first bad commit
commit c78bb4c4d89a8c17bb931675df6d39a87b9e32d3
Author: Steve Linton <steve.linton@st-andrews.ac.uk>
Date:   Thu Aug 17 16:06:27 2017 +0100

    Add String and DisplayString methods for functions
    A bit of a hack as it uses string streams rather than modifying the kernel
    Includes tests

:040000 040000 5573f8f6f728d34e80a3af86056e27779e0fc549 11ac47f8465cf180a013fae17020de06b454c049 M	lib
:040000 040000 424c39553c16341e5f338a841f4f86939875e504 10ab5039a5eec6a041c3e05b54afa954ef697244 M	tst
bisect run success
$

@ThomasBreuer
Copy link
Contributor

@fingolfin I am still running into error messages when my configure options include
something like with-gc=julia, which is not allowed in older versions.
Perhaps etc/bisect.sh should mention that one should switch to a setup without such options
before running the script?

@fingolfin
Copy link
Member Author

@ThomasBreuer running make will indeed re-run configure, with the options last used, and so yes, this won't work if you use options that are not supported by one of the tested commits. I can mention this in the documentation comment, but in the end, there are tons of caveats that one could add, and I cannot possible cover them all.

@fingolfin
Copy link
Member Author

Added a warning to the doc comment about rebuilding and configure options

Copy link
Contributor

@ThomasBreuer ThomasBreuer left a comment

Choose a reason for hiding this comment

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

I have no more general ideas for improving the documentation.
Thank you very much.

@ThomasBreuer ThomasBreuer merged commit e97e61e into gap-system:master Jan 28, 2019
@fingolfin fingolfin deleted the mh/bisect branch January 28, 2019 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants