| Submitter | Danek Duvall |
|---|---|
| Date | Aug. 21, 2013, 8:50 p.m. |
| Message ID | <20130821205014.GD2367@smelly.us.oracle.com> |
| Download | mbox | patch |
| Permalink | /patch/2236/ |
| State | Changes Requested |
| Headers | show |
Comments
On Wed, 2013-08-21 at 13:50 -0700, Danek Duvall wrote: > # HG changeset patch > # User Danek Duvall <danek.duvall@oracle.com> > # Date 1377115773 25200 > # Node ID 5e7ec26663958bc8fc51ee3ab517274268b4f498 > # Parent 6ac206fb6f27492a98f46bbff090407ee1b1de72 > test case fixes for Solaris > > Solaris versions of grep, diff, tail, and sed are different from the GNU > versions, causing some test cases to fail. Work around these differences > in OS-independent ways. Horrors. How did these persist so long? I think we probably shouldn't accept these without corresponding checks in contrib/check-code.py otherwise these will just be recurring problems that people on more mainstream Unices won't notice. Please separate out each class of breakage into a separate patch and include a corresponding check. Also, please start your patch summaries with a relevant keyword, like "solaris:".
Matt Mackall wrote: > On Wed, 2013-08-21 at 13:50 -0700, Danek Duvall wrote: > > # HG changeset patch > > # User Danek Duvall <danek.duvall@oracle.com> > > # Date 1377115773 25200 > > # Node ID 5e7ec26663958bc8fc51ee3ab517274268b4f498 > > # Parent 6ac206fb6f27492a98f46bbff090407ee1b1de72 > > test case fixes for Solaris > > > > Solaris versions of grep, diff, tail, and sed are different from the GNU > > versions, causing some test cases to fail. Work around these differences > > in OS-independent ways. > > Horrors. How did these persist so long? Because it's been a long time since I've done an upgrade of Mercurial in Solaris, and I haven't been running the bits in between myself (and I guess no one else is running the tests on Solaris, or cares). That's largely due to the need each time to upgrade this internal extension that we have, and fix up the man pages to work with the Solaris *roff tools, which is a tedious process, but I've made some changes to my build process that should make the latter significantly easier. I do want to set up automated nightly builds on Solaris, but it's been low on my priority list. > I think we probably shouldn't accept these without corresponding checks > in contrib/check-code.py otherwise these will just be recurring problems > that people on more mainstream Unices won't notice. Please separate out > each class of breakage into a separate patch and include a corresponding > check. Okay. Can I consider this confirmation that the workarounds I used were appropriate? I considered submitting some check-code fixes to find these, but had some questions: - I know that GNU head and tail at some point in the past started disallowing args like "-8" and +8", and required the use of -n, but then I think backed off of that change. So I'm not sure what the right thing is here. - I have no idea how to catch the problem in test-parse-date.t, since the contextual link between the missing newline and the sed that might not be able to recognize the last line of a file without it is tenuous. Is that something we can just leave to more frequent testing? > Also, please start your patch summaries with a relevant keyword, like > "solaris:". Sure. Thanks, Danek
On Wed, 2013-08-21 at 14:41 -0700, Danek Duvall wrote: > Matt Mackall wrote: > > > On Wed, 2013-08-21 at 13:50 -0700, Danek Duvall wrote: > > > # HG changeset patch > > > # User Danek Duvall <danek.duvall@oracle.com> > > > # Date 1377115773 25200 > > > # Node ID 5e7ec26663958bc8fc51ee3ab517274268b4f498 > > > # Parent 6ac206fb6f27492a98f46bbff090407ee1b1de72 > > > test case fixes for Solaris > > > > > > Solaris versions of grep, diff, tail, and sed are different from the GNU > > > versions, causing some test cases to fail. Work around these differences > > > in OS-independent ways. > > > > Horrors. How did these persist so long? > > Because it's been a long time since I've done an upgrade of Mercurial in > Solaris, and I haven't been running the bits in between myself (and I guess > no one else is running the tests on Solaris, or cares). I would have thought the Mercurial-on-Solaris community would be larger than the set using Cadmium. > > I think we probably shouldn't accept these without corresponding checks > > in contrib/check-code.py otherwise these will just be recurring problems > > that people on more mainstream Unices won't notice. Please separate out > > each class of breakage into a separate patch and include a corresponding > > check. > > Okay. Can I consider this confirmation that the workarounds I used were > appropriate? Well I can't say I was super-thrilled by them, as we've been trying to remove filtery-hackery. I'd rather just replace the problem bits with approaches that avoid the problem. Diff doesn't work portably? If the files are small, how about cat? But even where that's done, we'll want the check-code rule to avoid creep-back. > - I know that GNU head and tail at some point in the past started > disallowing args like "-8" and +8", and required the use of -n, but > then I think backed off of that change. So I'm not sure what the right > thing is here. Probably complains on some machines still. > - I have no idea how to catch the problem in test-parse-date.t, since the > contextual link between the missing newline and the sed that might not > be able to recognize the last line of a file without it is tenuous. Is > that something we can just leave to more frequent testing? > > > Also, please start your patch summaries with a relevant keyword, like > > "solaris:". > > Sure. > > Thanks, > Danek
Matt Mackall wrote: > I would have thought the Mercurial-on-Solaris community would be larger > than the set using Cadmium. I would expect so, too, but I just don't know. I guess another possibility is that they're putting /usr/gnu/bin first in $PATH, in which case this all runs just fine (at least on S11). > > > I think we probably shouldn't accept these without corresponding checks > > > in contrib/check-code.py otherwise these will just be recurring problems > > > that people on more mainstream Unices won't notice. Please separate out > > > each class of breakage into a separate patch and include a corresponding > > > check. > > > > Okay. Can I consider this confirmation that the workarounds I used were > > appropriate? > > Well I can't say I was super-thrilled by them, as we've been trying to > remove filtery-hackery. I'd rather just replace the problem bits with > approaches that avoid the problem. Diff doesn't work portably? If the > files are small, how about cat? Seems to be "diff -u" specifically when there's no difference. We could make the rule that when you expect no difference, you use plain "diff", or "cmp", or something, though of course when something goes wrong, and there *is* a difference, you either get less useful output (context diffs) or no output at all (cmp). test-command-template.t uses both cmp and diff, running diff only if cmp finds differences. I could write python code using difflib, but that seems like overkill. I probably would gravitate towards using context diffs for no output (assuming that you can write a test that checks both the command and its output), or the cmp/diff combo if I can figure out that regex. But I'm not sure I'm qualified to make the decision here. I'm really not sure how else to work around the lack of grep -a. > But even where that's done, we'll want the check-code rule to avoid > creep-back. > > > - I know that GNU head and tail at some point in the past started > > disallowing args like "-8" and +8", and required the use of -n, but > > then I think backed off of that change. So I'm not sure what the right > > thing is here. > > Probably complains on some machines still. Right. So if we have to deal with both, then we need an alternative to tail. You can use "sed -n 'x,$p'" as an alternative to "tail +x", but I'm not sure how to emulate the "normal" tail operation with other shell commands. Again, I could write this in python, but ... So I'd welcome your (or anyone else's) suggestions. Thanks, Danek
On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote: > Seems to be "diff -u" specifically when there's no difference. We could > make the rule that when you expect no difference, you use plain "diff", or > "cmp", or something, though of course when something goes wrong, and there > *is* a difference, you either get less useful output (context diffs) or no > output at all (cmp). test-command-template.t uses both cmp and diff, > running diff only if cmp finds differences. I'm not super-worried about the case where the test fails and we get weird output. > I could write python code using difflib, but that seems like overkill. Indeed. > I probably would gravitate towards using context diffs for no output > (assuming that you can write a test that checks both the command and its > output), or the cmp/diff combo if I can figure out that regex. But I'm not > sure I'm qualified to make the decision here. > > I'm really not sure how else to work around the lack of grep -a. Perhaps use our inline Python support: $ foo > bar >>> for line in open('bar'): ... if 'baz' in line: print line > > But even where that's done, we'll want the check-code rule to avoid > > creep-back. > > > > > - I know that GNU head and tail at some point in the past started > > > disallowing args like "-8" and +8", and required the use of -n, but > > > then I think backed off of that change. So I'm not sure what the right > > > thing is here. > > > > Probably complains on some machines still. > > Right. So if we have to deal with both, then we need an alternative to > tail. You can use "sed -n 'x,$p'" as an alternative to "tail +x", but I'm > not sure how to emulate the "normal" tail operation with other shell > commands. Looks like the two places that are using it are legacy from before we had glob/regex support built into the test format.
Matt Mackall wrote: > On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote: > > Seems to be "diff -u" specifically when there's no difference. We could > > make the rule that when you expect no difference, you use plain "diff", or > > "cmp", or something, though of course when something goes wrong, and there > > *is* a difference, you either get less useful output (context diffs) or no > > output at all (cmp). test-command-template.t uses both cmp and diff, > > running diff only if cmp finds differences. > > I'm not super-worried about the case where the test fails and we get > weird output. Okay. > > I probably would gravitate towards using context diffs for no output > > (assuming that you can write a test that checks both the command and its > > output), or the cmp/diff combo if I can figure out that regex. But I'm not > > sure I'm qualified to make the decision here. > > > > I'm really not sure how else to work around the lack of grep -a. > > Perhaps use our inline Python support: > > $ foo > bar > >>> for line in open('bar'): > ... if 'baz' in line: print line Nice; done. > > > But even where that's done, we'll want the check-code rule to avoid > > > creep-back. > > > > > > > - I know that GNU head and tail at some point in the past started > > > > disallowing args like "-8" and +8", and required the use of -n, but > > > > then I think backed off of that change. So I'm not sure what the right > > > > thing is here. > > > > > > Probably complains on some machines still. > > > > Right. So if we have to deal with both, then we need an alternative to > > tail. You can use "sed -n 'x,$p'" as an alternative to "tail +x", but I'm > > not sure how to emulate the "normal" tail operation with other shell > > commands. > > Looks like the two places that are using it are legacy from before we > had glob/regex support built into the test format. Not sure I follow how that would allow us to pipe a set of N lines of a command's output into a file. In particular, I don't see how tail's use in test-import-merge.t can be converted to that. I'll also note that "tail -2" is already used in test-copy.t (changed to remove -n back in 2006, apparently on request from a Sun engineer) and hasn't caused any problems. Plus "head" is used in numerous places without -n. Though I'll note that on Solaris, for some idiotic reason /usr/bin/head supports -n, while /usr/bin/tail doesn't. It's been that way as far back as I can tell (1994 or so), so I suppose it's possible that for whatever time period the GNU versions required -n, they may have only done so for one command and not the other for similarly inscrutable reasons. At any rate, given the rampant disuse of -n, should I just leave this change alone and add a test to prevent it from coming back? Thanks, Danek
Danek Duvall wrote: > Matt Mackall wrote: > > > On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote: > > > Seems to be "diff -u" specifically when there's no difference. We could > > > make the rule that when you expect no difference, you use plain "diff", or > > > "cmp", or something, though of course when something goes wrong, and there > > > *is* a difference, you either get less useful output (context diffs) or no > > > output at all (cmp). test-command-template.t uses both cmp and diff, > > > running diff only if cmp finds differences. > > > > I'm not super-worried about the case where the test fails and we get > > weird output. > > Okay. Oh, but I'm still not sure what to do for a test here, as "diff -u" is just fine as long as there's output; it's only if there is none that it's problematic. Usage is all over the place: - three instances of using "cmp" before "diff -u" - one instance of piping "diff -u" through "grep" - one instance of actually expecting output - three instances of expecting no output (I missed one, since the bzr tests aren't run on my machine) Seems like the simplest testable option is to convert all calls to diff to not use -u, remove all the cmp/grep calls, and convert the one bit of output to be context diffs. Does that work for you? Thanks, Danek
Danek Duvall wrote: > Danek Duvall wrote: > > > Matt Mackall wrote: > > > > > On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote: > > > > Seems to be "diff -u" specifically when there's no difference. We could > > > > make the rule that when you expect no difference, you use plain "diff", or > > > > "cmp", or something, though of course when something goes wrong, and there > > > > *is* a difference, you either get less useful output (context diffs) or no > > > > output at all (cmp). test-command-template.t uses both cmp and diff, > > > > running diff only if cmp finds differences. > > > > > > I'm not super-worried about the case where the test fails and we get > > > weird output. > > > > Okay. > > Oh, but I'm still not sure what to do for a test here, as "diff -u" is just > fine as long as there's output; it's only if there is none that it's > problematic. Usage is all over the place: > > - three instances of using "cmp" before "diff -u" > > - one instance of piping "diff -u" through "grep" > > - one instance of actually expecting output > > - three instances of expecting no output (I missed one, since the bzr tests > aren't run on my machine) > > Seems like the simplest testable option is to convert all calls to diff to > not use -u, remove all the cmp/grep calls, and convert the one bit of > output to be context diffs. Does that work for you? Dammit, I need to investigate more before hitting send. The one instance in test-glog.t of diff -u is in a function which is used dozens of times, numerous ones with actual output. Should I convert them all? Thanks, Danek
On Thu, 2013-08-22 at 16:13 -0700, Danek Duvall wrote: > Danek Duvall wrote: > > > Matt Mackall wrote: > > > > > On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote: > > > > Seems to be "diff -u" specifically when there's no difference. We could > > > > make the rule that when you expect no difference, you use plain "diff", or > > > > "cmp", or something, though of course when something goes wrong, and there > > > > *is* a difference, you either get less useful output (context diffs) or no > > > > output at all (cmp). test-command-template.t uses both cmp and diff, > > > > running diff only if cmp finds differences. > > > > > > I'm not super-worried about the case where the test fails and we get > > > weird output. > > > > Okay. > > Oh, but I'm still not sure what to do for a test here, as "diff -u" is just > fine as long as there's output; it's only if there is none that it's > problematic. Usage is all over the place: > > - three instances of using "cmp" before "diff -u" Don't care. > - one instance of piping "diff -u" through "grep" Ugh. > - one instance of actually expecting output This sort of thing can be replaced with cat as the files are one line. > - three instances of expecting no output (I missed one, since the bzr tests > aren't run on my machine) Ok. > Seems like the simplest testable option is to convert all calls to diff to > not use -u, remove all the cmp/grep calls, and convert the one bit of > output to be context diffs. Does that work for you? Sounds good.
Patch
diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t --- a/tests/test-hgweb-commands.t +++ b/tests/test-hgweb-commands.t @@ -1256,8 +1256,8 @@ commit message with Japanese Kanji 'Noh' Graph json escape of multibyte character $ "$TESTDIR/get-with-headers.py" 127.0.0.1:$HGPORT 'graph/' \ - > | grep -a '^var data =' - var data = [["061dd13ba3c3", [0, 1], [[0, 0, 1, -1, ""]], "\\u80fd", "test", "1970-01-01", ["unstable", true], ["tip"], ["something"]], ["cad8025a2e87", [0, 1], [[0, 0, 1, 3, "FF0000"]], "branch commit with null character: \x00", "test", "1970-01-01", ["unstable", false], [], []], ["1d22e65f027e", [0, 1], [[0, 0, 1, 3, ""]], "branch", "test", "1970-01-01", ["stable", true], [], []], ["a4f92ed23982", [0, 1], [[0, 0, 1, 3, ""]], "Added tag 1.0 for changeset 2ef0ac749a14", "test", "1970-01-01", ["default", true], [], []], ["2ef0ac749a14", [0, 1], [], "base", "test", "1970-01-01", ["default", false], ["1.0"], ["anotherthing"]]]; (esc) + > | cat -v | grep '^var data =' + var data = [["061dd13ba3c3", [0, 1], [[0, 0, 1, -1, ""]], "\\u80fd", "test", "1970-01-01", ["unstable", true], ["tip"], ["something"]], ["cad8025a2e87", [0, 1], [[0, 0, 1, 3, "FF0000"]], "branch commit with null character: ^@", "test", "1970-01-01", ["unstable", false], [], []], ["1d22e65f027e", [0, 1], [[0, 0, 1, 3, ""]], "branch", "test", "1970-01-01", ["stable", true], [], []], ["a4f92ed23982", [0, 1], [[0, 0, 1, 3, ""]], "Added tag 1.0 for changeset 2ef0ac749a14", "test", "1970-01-01", ["default", true], [], []], ["2ef0ac749a14", [0, 1], [], "base", "test", "1970-01-01", ["default", false], ["1.0"], ["anotherthing"]]]; (esc) capabilities diff --git a/tests/test-highlight.t b/tests/test-highlight.t --- a/tests/test-highlight.t +++ b/tests/test-highlight.t @@ -522,7 +522,7 @@ hgweb fileannotate, raw $ echo "" >> b $ echo "" >> b $ echo "" >> b - $ diff -u b a + $ diff -u b a | grep '^[-+@ ]' || : hgweb filerevision, raw @@ -531,7 +531,7 @@ hgweb filerevision, raw $ echo "200 Script output follows" > b $ echo "" >> b $ hg cat primes.py >> b - $ diff -u b a + $ diff -u b a | grep '^[-+@ ]' || : hgweb highlightcss friendly diff --git a/tests/test-import-merge.t b/tests/test-import-merge.t --- a/tests/test-import-merge.t +++ b/tests/test-import-merge.t @@ -129,7 +129,7 @@ Test that --exact on a bad header doesn' $ echo a>>a $ hg ci -m3 $ hg export 2 | head -7 > ../a.patch - $ hg export tip | tail -n +8 >> ../a.patch + $ hg export tip | tail +8 >> ../a.patch $ cd .. $ hg clone -qr0 repo3 repo3-clone diff --git a/tests/test-parse-date.t b/tests/test-parse-date.t --- a/tests/test-parse-date.t +++ b/tests/test-parse-date.t @@ -242,7 +242,7 @@ Test issue 3764 (interpreting 'today' an >>> yesterday = (datetime.date.today() - datetime.timedelta(days=1)).strftime("%b %d") >>> dates = open('dates', 'w') >>> dates.write(today + '\n') - >>> dates.write(yesterday) + >>> dates.write(yesterday + '\n') >>> dates.close() $ hg ci -d "`sed -n '1p' dates`" -m "today is a good day to code" $ hg log -d today --template '{desc}\n'