usr.sbin/etcupdate/preworld_test:main started failing on https://jenkins.freebsd.org/job/FreeBSD_HEAD/471/ . r303047 seems like the most likely culprit. Repro: Run `kyua debug -k /usr/tests/usr.sbin/etcupdate/Kyuafile preworld_test:main` as root, e.g. $ sudo kyua debug -k /usr/tests/usr.sbin/etcupdate/Kyuafile preworld_test:main Differences for -n: Differences for real: Checking tree for correct results: File /etc/master.passwd has wrong contents File /etc/group has wrong contents File /etc/passwd should be a regular file File /etc/pwd.db should be a regular file File /etc/spwd.db should be a regular file preworld_test:main -> failed: Returned non-success exit status 1
(In reply to Ngie Cooper from comment #0) Confirmed -- r303047 is the culprit: $ (set -e; cd /usr/src/svn/usr.bin/sed/; svn merge -c -r303047 .; make obj; make depend; make all; sudo make install) ... $ sudo kyua debug -k /usr/tests/usr.sbin/etcupdate/Kyuafile preworld_test:main Differences for -n: Differences for real: Checking tree for correct results: preworld_test:main -> passed
sed's succeeding... it's a change in behavior somehow that wouldn't have been caught by the etcupdate tests if that had used `atf_check ... sed ...` instead of `sed ...`: $ sudo kyua debug -k /usr/tests/usr.sbin/etcupdate/Kyuafile preworld_test:main 0 0 0 Differences for -n: Differences for real: Checking tree for correct results: File /etc/master.passwd has wrong contents File /etc/group has wrong contents File /etc/passwd should be a regular file File /etc/pwd.db should be a regular file File /etc/spwd.db should be a regular file preworld_test:main -> failed: Returned non-success exit status 1
Mikhail can you please take a look? We can revert r303047 (and r303146) if needed.
-i vs -I isn't the issue...
Here's the difference: root:<rpass>:0:0::0:0:Charlie &:/root:/bin/csh^M toor:*:0:0::0:0:Bourne-again Superuser:/root:^M daemon:*:1:1::0:0:Owner of many system processes:/root:/usr/sbin/nologin^M operator:*:2:5::0:0:System &:/:/usr/sbin/nologin^M _dhcp:*:65:65::0:0:dhcp programs:/var/empty:/usr/sbin/nologin^M uucp:*:66:66::0:0:UUCP pseudo-user:/var/spool/uucppublic:/usr/local/libexec/uucp/uucico^M pop:*:68:6::0:0:Post Office Owner:/nonexistent:/usr/sbin/nologin^M auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin^M www:*:80:80::0:0:World Wide Web Owner:/nonexistent:/usr/sbin/nologin^M hast:*:845:845::0:0:HAST unprivileged user:/var/empty:/usr/sbin/nologin^M nobody:*:65534:65534::0:0:Unprivileged user:/nonexistent:/usr/sbin/nologin^M john:<password>:1001:1001::0:0:John Baldwin:/home/john:/bin/tcsh^M messagebus:*:556:556::0:0:D-BUS Daemon User:/nonexistent:/usr/sbin/nologin^M polkit:*:562:562::0:0:PolicyKit User:/nonexistent:/usr/sbin/nologin^M haldaemon:*:560:560::0:0:HAL Daemon User:/nonexistent:/usr/sbin/nologin^M #^M root:<rpass>:0:0::0:0:Charlie &:/root:/bin/csh^M toor:*:0:0::0:0:Bourne-again Superuser:/root:^M daemon:*:1:1::0:0:Owner of many system processes:/root:/usr/sbin/nologin^M operator:*:2:5::0:0:System &:/:/usr/sbin/nologin^M _dhcp:*:65:65::0:0:dhcp programs:/var/empty:/usr/sbin/nologin^M uucp:*:66:66::0:0:UUCP pseudo-user:/var/spool/uucppublic:/usr/local/libexec/uucp/uucico^M pop:*:68:6::0:0:Post Office Owner:/nonexistent:/usr/sbin/nologin^M auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologinwww:*:80:80::0:0:World Wide Web Owner:/nonexistent:/usr/sbin/nologin^M hast:*:845:845::0:0:HAST unprivileged user:/var/empty:/usr/sbin/nologin^M nobody:*:65534:65534::0:0:Unprivileged user:/nonexistent:/usr/sbin/nologin^M john:<password>:1001:1001::0:0:John Baldwin:/home/john:/bin/tcsh^M messagebus:*:556:556::0:0:D-BUS Daemon User:/nonexistent:/usr/sbin/nologin^M polkit:*:562:562::0:0:PolicyKit User:/nonexistent:/usr/sbin/nologin^M haldaemon:*:560:560::0:0:HAL Daemon User:/nonexistent:/usr/sbin/nologin^M #^M wheel:*:0:root,john^M daemon:*:1:^M kmem:*:2:^M sys:*:3:^M tty:*:4:^M operator:*:5:root^M _dhcp:*:65:^M uucp:*:66:^M dialer:*:68:^M network:*:69:^M audit:*:77:^M www:*:80:^M hast:*:845:^M nogroup:*:65533:^M nobody:*:65534:^M john:*:1001:^M messagebus:*:556:^M polkit:*:562:^M haldaemon:*:560:^M vs #^M wheel:*:0:root,john^M daemon:*:1:^M kmem:*:2:^M sys:*:3:^M tty:*:4:^M operator:*:5:root^M _dhcp:*:65:^M uucp:*:66:^M dialer:*:68:^M network:*:69:^M audit:*:77:www:*:80:^M hast:*:845:^M nogroup:*:65533:^M nobody:*:65534:^M john:*:1001:^M messagebus:*:556:^M polkit:*:562:^M haldaemon:*:560:^M A simplified testcase: $ echo "www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin" | sed -e '/:80:/i\ auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin' auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologinwww:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin
(In reply to Ngie Cooper from comment #5) > A simplified testcase Ngie, it is a little hard to distinguish Bugzilla's line-wrappings from the intended EOLs... Here is what I just tested: % cat t-211399 /:80:/i\ auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin % echo "www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin" | \ sed -f t-211399 auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin Using gsed instead of sed produces the exact same output -- which seems correct. Could you, please, refine the example? That said, I'm testing on 10.x -- where I've developed the patch in Bug #195929. Maybe, the -current got a slightly different change, which would account for the differences...
(In reply to Mikhail Teterin from comment #6) Just checking ..we don't have FreeBSD-12 reference machines yet :(. The version from the current tree should build just fine on FreeBSD 10/11 for testing purposes though.
(In reply to Ngie Cooper from comment #5) So .. two questions: Is this a real issue(TM)? It would appear the new files are ase usable as the original ones .. but maybe I am missing something. What does GNU sed do on this test?
The key difference is that gsed and versions of sed prior to r303047 printed out the newline separating the password entry for auditdistd and www. sed after r303047 however omits that newline. Examples below: CentOS 6.5: % echo "www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin" | sed -e '/:80:/i\ auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin' auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin % cat /etc/redhat-release CentOS release 6.5 (Final) FreeBSD after r303047: # echo "www:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin" | sed -e '/:80:/i\ > auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologin' auditdistd:*:78:77::0:0:Auditdistd unprivileged user:/var/empty:/usr/sbin/nologinwww:*:80:80:World Wide Web Owner:/nonexistent:/usr/sbin/nologin # uname -a FreeBSD fattw-3-2 12.0-CURRENT FreeBSD 12.0-CURRENT #12 r303199+8d3c361(isilon-atf): Fri Jul 22 13:41:49 PDT 2016 ngie@wkstn-fbsd-ngie:/usr/obj/usr/src/sys/GENERIC amd64 Something seems to have changed on FreeBSD CURRENT -- some older custom versions of FreeBSD I have access to (based on r29xxxxx) don't necessarily exhibit this issue. I need to dig through a bit further to figure out why.
(In reply to Ngie Cooper from comment #9) There have been many changes in before r303047 (all are in 11-stable) to make our sed is somewhat more similar to gsed without hurting standards. r302973 was not fully tested and may need further reviews but if the tests pass in 11-stable that is not the culprit. If reverting r303047 (which is only in -current) is the only requirement to keep consistency with gsed and previous sed, let's do it.
If the problem is a missing \n, I may have an idea. But would still appreciate a concise test case. Can the revert wait for a couple of days? Thanks!
(In reply to Mikhail T. from comment #11) Yeah, we can wait ... thanks for looking at it.
Created attachment 173122 [details] Test-case Ok, here is the simpler test-case... gsed and older seds all print out two lines: abcd 1234 The new sed prints out one line: abcd1234 Now, this is a bug simply because it is _different_ from the established behavior. But I wonder, if the established behavior is correct -- if the closing quote on sed's command-line is on a new line, the new sed will print the two lines. The old seds do not change their behavior regardless of where the closing quote is. So the new code gives the coder a choice, whereas the old one did not... Both the old and the new sed will ignore any _additional_ new lines before the closing quote. I'll try to fix it now.
(In reply to Mikhail T. from comment #13) Do check the code ngie@ added in phabricator: it's in the URL for this PR.
Created attachment 173125 [details] Fix and then some > Do check the code ngie@ added in phabricator Ngie's proposal is a work-around, not a fix. He unconditionally adds \n to the argument, duplicating it entirely in the process :) The full patch, attached, fixes the problem and also makes use of the tlen-field to avoid calling strlen() in process.c. I change the type of tlen from size_t to int, because that's what printf wants to see. I'd like us to begin using the lengths already set by compile.c so that, some day, the lines may come straight from the mmap-ed script instead of being copies. The bare minimum required to fix the problem in the PR is just one hunk from the above: Index: compile.c =================================================================== --- compile.c (revision 303554) +++ compile.c (working copy) @@ -753,7 +753,10 @@ err(1, "realloc"); } } - text[size] = '\0'; + /* Ensure, there is a new line at the end: */ + if (text[size - 1] != '\n') { + text[size++] = '\n'; + } if ((text = realloc(text, size + 1)) == NULL) err(1, "realloc"); *ptlen = size; Both ours (multi_test_ and GNU's test-suits pass with either the above minimum hunk or the bigger patch in the attachment.
Err, sorry, the minimum hunk is not enough by itself -- not without the terminating '\0'. If you wish, I can come up with the smaller patch, but the attachment #173125 [details] is preferable.
(In reply to Mikhail T. from comment #16) I do prefer Mikhail's approach, ngie can you confirm/commit?
(In reply to Pedro F. Giffuni from comment #17) The attached patch doesn't work. The way I attached to phabricator is the only way I know of (so far) that fixes the behavior to not regress from the pre-r303047 (all I did in the phabricator review was restore the code in a less hacky way by using asprintf instead of malloc+strcpy+strcat). $ sudo kyua -k /usr/tests/usr.sbin/etcupdate/tests/Kyuafile always_test:main -> passed [1.805s] conflicts_test:main -> passed [0.379s] fbsdid_test:main -> passed [0.424s] ignore_test:main -> passed [0.153s] preworld_test:main -> failed: Returned non-success exit status 1 [0.147s] tests_test:main -> passed [1.988s] tzsetup_test:main -> passed [0.169s] Results file id is usr_tests_usr.sbin_etcupdate.20160731-004120-932624 Results saved to /root/.kyua/store/results.usr_tests_usr.sbin_etcupdate.20160731-004120-932624.db 6/7 passed (1 failed) *** Error code 1 Stop. make: stopped in /usr/src/svn/usr.sbin/etcupdate/tests $
(In reply to Ngie Cooper from comment #18) > The attached patch doesn't work. Could you post an actual test-case, please? Or is the one I included (attachment #173122 [details]) still failing in your environment? Thanks!
(In reply to Mikhail T. from comment #19) - A repro is provided in comment # 0 using kyua debug. - The patch you provided is failing the test shown in comment # 0. This has been broken for 10 days now and has been spamming freebsd-current@ with failure emails. I will commit my change noted in phabricator and we can work out "making it better" later.
A commit references this bug: Author: ngie Date: Sun Jul 31 05:31:10 UTC 2016 New revision: 303572 URL: https://svnweb.freebsd.org/changeset/base/303572 Log: Fix regression with /i caused by r303047 '\n' was specifically added to -e arguments prior to r303047. Restore historical behavior which in turn fixes usr.sbin/etcupdate/preworld_test:main . The fix is being committed to address the issue in the short term and may be iterated upon as noted in bug 211399 Discussed with: mi, pfg Differential Revision: https://reviews.freebsd.org/D7368 PR: 195929, 211399 [*] MFC after: 18 days X-MFC with: r303047 Reported by: Jenkins Sponsored by: EMC / Isilon Storage Division Changes: head/usr.bin/sed/main.c
(In reply to Ngie Cooper from comment #20) > - A repro is provided in comment # 0 using kyua debug. No, that's not a test-case. You offered one in comment #5, which I trimmed further down into attachment #173122 [details]. Unfortunately, you chose to ignore the question I asked about it in comment #19 and took this less than cooperative attitude. I ask the question again: is the script I posted as attachment #173122 [details] producing joined (incorrect) or separate (correct) lines of output, when given a path to the sed-executable built with patch in attachment #173125 [details] applied (but without your base r303572)? Thank you.
Hasn't failed for some time. Closing.