Vladimir Kotal wrote: > Roland Mainz wrote: > > Can you please take a quick look at > > http://cr.opensolaris.org/~gisburn/cr6842892_webrev_cleanup_002/ and > > check whether there are any problems with the patch ? > > <snip> > > > - Add builtin manpage via --man (the same way as I did the --man support > > for "bldenv"). The seperate webrev.1 file in the sources will be removed > > in the next webrev and instead the build will use $ webrev --nroff # on > > the script to get the *roff source for the manpage (this should lower > > maintaince since "getopts" string and manpage are now one string/blob). > > I am not sure I follow. The webrev for this change (what is the CR > number, by the way ?) dispays just webrev.sh. It looks like the removal > of webrev.1 is missing in the changeset.
Right... that's why I said "Quick preliminary review" since I simply hacked the webrev.sh code and initially didn't care about the matching Makefile infratructure > Also, I have 3 concerns regarding this particular change: > - it seems it's time to stop the explosion of the script in terms of > space; it's way too long already. Erm... it's possible to split the code into modules (either in the sane source file or in seperate source files, for example one per class (see "class" comment below)). I agree that the code is a bit messy since it grew and grew over time and was never modularised. That's why I'd like to fix soon like this: 1. Wrap per-source file webrev generation into a function (which can then run in parallel (which covers I/O-latency)) 2. Wrap index generation into a function 3. Turn various functionality into ksh93 "interfaces" (ksh93 has an object-oriented type system (short: Try thinking at the lines of JAVA "class" and "interface"), see typeset -T=(...) ), for example the webrev upload and SCM interfacing (e.g. there would be one SCM "interface" with "implementations" for "Mercurial", "teamware", "subvesion"). That will eliminate lots of global variables which are spread over all the code and help maintaining the code in the long-term > - is the generated man page really the same as the old one ? Maybe > you could produce a diff of the original man page and the man page > generated from the 'USAGE' field ? The generated manpage in the new webrev is the same as the original one, I simply used an ASCII dump to create the builtin manpage and then compared it by hand. Duing a diff via /usr/bin/diff is more difficult since the spacing and layout "shifts" between both versions and /usr/bin/diff is unsuited for such comparisations. > - not sure if sucking the man page into the script is the right thing > to do Erm... the idea is to have the manpage integrated into the script to make sure they always stay in sync without having to change always two source files (and the current webrev.1 requires that people still remeber the nroff syntax) - and as a side-effect the usual AST/ksh93 "getopts" goodies apply, e.g. $ webrev --man # will print a manpage, $ webrev --help # a summary of the options, $ webrev --version # version information etc. - which all helps users, deployment and bug reporting... > (say the script will be rewritten into Python one day Why should a rewrite in python be "better" ? I would say that a properly-written ([1]) ksh93 script can easily outperform a python script, assuming the python script has to use the same external commands for doing the file lookup and file diff'ing and both ksh93 and the python interpreter run in a multibyte locale. [1]=Right now the script suffers from lots of problems, primariry caused by the problem that the script is still mostly written like Bourne-shell scripts 20 years ago (except recent additions from the last two years), for example: - the script |fork()|'s children like mad, sometimes even one per line (e.g. the "urlencoder" was one example - I've killed that in my new version) - the script uses temporary files excessively, even when the matching information could be stored in an array - the script uses external commands and pipes where it can be avoided. Half of the awk+sed usage should really be replaced and the use of "self-modifying" script code should really be removed (it's a) slow and b) defeats ksh93's builtin script compiter+caches in horrible ways) - the script causes massive |stat()| calls by excessive usage of the "-x"/"-f" test operators. Most of this could be avoided by using variable trees which store the information _once_ - script causes lots of |open()|/|seek(SEEK_END)|/|write()|/|close()|-sequences per file, which makes webrev unusually slow on high-latency filesystems (e.g. NFS). That culd easily be avoided by using dedicated per-file file descriptors instead of "> filename"/">> filename"-style redirection > ). Why is > that necessary ? It isn't neccesarty but I think it's usefull... we already did that for "bldenv.sh" and other stuff. If the manpage should really kept as seperate file I'd vote to turn it into a DocBook/XML-based manpage since this can be used to generate nroff, plain text, PDF and AST getopts formats (the last functionality isn't completely done yet... ;-( ). Anyway... new webrev is available as http://cr.opensolaris.org/~gisburn/cr6842892_webrev_cleanup_002/ ---- Bye, Roland -- __ . . __ (o.\ \/ /.o) roland.ma...@nrubsig.org \__\/\/__/ MPEG specialist, C&&JAVA&&Sun&&Unix programmer /O /==\ O\ TEL +49 641 3992797 (;O/ \/ \O;) _______________________________________________ tools-discuss mailing list tools-discuss@opensolaris.org
