Mark J. Nelson wrote:
> Vladimir Kotal wrote:
>> Vladimir Kotal wrote:
>>> Hi all,
>>>
>>> Based on feedback from Dan Price, I've changed webrev.sh a bit to
>>> make it more pleasant to work with (in terms of webrev upload) and to
>>> fix 2 small bugs.
>>>
>>> webrev is here:
>>> http://cr.opensolaris.org/~vkotal/webrev-better_upload.onnv/
>>
>> I know everyone is busy with more funny/interesting stuff but this one
>> still needs some review.
>
> Have you tested all of the output paths? It seems like you're using
> trailing spaces inconsistently, such that some messages will not have
> spaces in between them?
I believe most of the output paths were tested and they seemed fine.
However, after playing with the script a bit more, it became obvious
that error output should be better (because it collided with the single
line output) so I modified it a bit. Now if some of the upload commands
fails the error will be nicely printed (i.e. not in the middle of webrev
output).
E.g. for SSH transport:
$ webrev -n -U -t ssh://cr.opensolAaris.org:test-43
SCM detected: mercurial
File list from: hg-active -p /ws-local/onnv-clone ... Done.
Upload to: ssh://cr.opensolAaris.org:test-43/
Uploading: rmdir upload ... Failed
ERROR: scp failed
src dir: '/builds/vk199839/webrev-better_upload.onnv/webrev'
dst dir: 'cr.opensolAaris.org:test-43'
error messages:
> ssh: cr.opensolAaris.org: node name or service name not known
> lost connection
$ echo $?
1
And for the failover case:
$ RSYNC=/bin/false webrev -U -O -o $CODEMGR_WS/webrev-failover
SCM detected: mercurial
File list from: hg-active -p /ws-local/onnv-clone ... Done.
Workspace: /builds/vk199839/webrev-better_upload.onnv (at
d04ce5ecbe7d)
Compare against: /ws-local/onnv-clone (at 1eb5efc243ad)
Output to: /builds/vk199839/webrev-better_upload.onnv/webrev-failover
Output Files:
usr/src/tools/scripts/webrev.sh
patch cdiffs udiffs wdiffs sdiffs frames ps old new
Generating PDF: Done.
index.html: Done.
Upload to: rsync://cr.opensolaris.org:webrev-failover/
Uploading: rsync ... Failed. (falling back to SSH)
Upload to: ssh://cr.opensolaris.org:webrev-failover/
Uploading: rmdir upload ... Done.
$ echo $?
0
I have added SED variable in the process along with related substitutions.
> What testing did you do for the remote target specification validation?
It's only so much that can be tested given it now only checks invalid
prefix and missing colon.
Apart from the unit tests [*] I have played a bit with the new script.
I have actually spent some time thinking about making the validation
more strict, by checking the remote host and see if it maps to valid IP
address but it gets complicated because the host part could be also IP
address (not talking about the IPv6 address notation with square
brackets) so it ended up like this (better rudimentary checking for most
common errors than nothing).
> Does scp not have a trailing-slash problem like rsync? (Ie it seems
> like webrev should be using "$WDIR/*" instead of "$WDIR" as the src arg?)
scp deals with the trailing slash just fine.
The following examples will do the right thing:
- without -o/-t
scp -r $CODEMGR_WS/webrev cr.opensolaris.org:bugfix-123
- with -o:
scp -r $CODEMGR_WS/custom_dir cr.opensolaris.org:custom_dir
In the case with "$WDIR/*" I'd have to create remote directory first
(via SFTP) for the simplistic case as well (as opposed to the current
way where SFTP is only used for the case with more than one remote
directory) which would unnecessarily slow down already slow SSH
uploading process.
> Seems like lines 349 and 354 could be rolled together.
Sure. Now done inside print_upload_header().
> Aren't you losing the return code from ssh_upload now, on line 375-378?
D'oh, yes; it was done not very carefully before but now it is real
issue. Fixed.
In the course of testing the changes I have discovered CR 6807228 so
there is now a fix for it too in the webrev.
Incremental webrev is here:
http://cr.opensolaris.org/~vkotal/webrev-better_upload.onnv.2nd/
Up-to-date webrev against ON clone is here:
http://cr.opensolaris.org/~vkotal/webrev-better_upload.onnv.final/
v.
[*]
http://cr.opensolaris.org/~vkotal/webrev-better_upload.onnv-unit-tests.txt
