[illumos-Developer] Requesting code review for opensource /usr/xpg4/bin/more replacement with "less" ...

[Roland Mainz]

On Thu, Jan 20, 2011 at 8:55 PM, Gordon Ross <Gordon.Ross at nexenta.com> wrote:
> I would go with plain regex, as that's what less uses now,
> as well as xpg4/bin/more.

Yes, we asked around... while a better and more capable regex engine
would IMO be very nice the correct way may be to enhance the current
libc implementation for now. I'll defer that issue until we can make a
meeting about that.

> Builds under usr/src/cmd normally don't use the separate
> compile directories for each target (i386, sparc).

Erm... this layout was chosen intentionally:
1. Many new additions in usr/src/cmd/ already use seperate directories
for source and object files
2. "less" is maintained by an active upstream and has a different
license. IMO a clean approach which seperates the platform-independent
sources into a common/-subdirectory and platform-specific
souces+object files into another directory is cleaner in this case and
makes maintaince a bit easier.
3. "defines.h" needs to be per-architecture and therefore either
per-architecture subdirs or patching the upstream sources becomes
neccesary to handle this (and I'm not happy about patching the
upstream sources (unless this is properly documented (and IMO
unavoidable)))

> Please put the sources directly under usr/src/cmd/less
> and use one Makefile that builds for the default target.
> (i386, sparc)  No need for 64-bit here, btw.

Erm... I disagree: 32bit on a 64bit kernel looks very weired for me -
effectivly it's an _artificial_ capabilty downgrade (and "no" ...
there is no performance penalty for using 64bit instead of 32bit. On
x86 64big (=AMD64) is even faster than 32bit) and on SPARC the
original mythos "... about 64bit applications being slower than
32bit..." came from old+buggy Sun Workshop versions doing a poor
compile jobs for UltraSPARC-1/2 targets and some
pathological+artificial testcases.
Additionally using 64bit by default helps a bit to keep the codebase
64bit clean, something which is IMHO urgendly needed in OS/Net (unless
we never want any ports (which IMO would lead to the slow death of
Illumos)).

[snip]
> What was the reason for adding these?
>  -D_XOPEN_SOURCE=600 -D__EXTENSIONS__=1
> Looks like the existing one (from SFW) did not use those.
> Did you observe any actual POSIX conformance problems
> when less is built without those flags?

It's per definition:
1. All utilties whichi go into /usr/xpg6/bin/ and/or /usr/xpg4/bin
must be compiled with XPG6/XPG4 flags (and usually the higher standard
version comes first).
2. Specifically "less" uses at least one of the libc functions which
comes in both plain and XPG4 flavors: |system()|. The SFWNV build
never cared about standards conformance and just stuck with the libc
default while our build in OS/Net tries to conform to the POSIX/SUS
definition and therefore must use the XPG version of |system()| and
not the plain one

> I believe if you instead clear CCVERBOSE,
> you don't need to set all those ERRWARN flags.

I did that intentionally to document that we have warnings which need
to be fixed. I want to remove this stuff ASAP when we pull a new
version from upstream which has the warning fixed (see above and below
- it's about long-term maintainablity vs. short-term
patch-to-get-rid-of-the-itching-and-then-no-longer-care-about-the-disease
solution).

> And in general, try to make this more like other
> usr/src/cmd/* Makefiles.

Erm... that's what I tried to do... but I also try to have a clean
approach and keep platform-independent sources and object
files+platform-dependent sources seperated.

> * usr/src/cmd/less/screen.c
> errors out when I try to compile it with the
> normal ON build flags (-w no implicit)
> I would just add the four missing declarations.

I would really prefer not to patch the upstream sources until it
becomes unavoidable and instead push a patch into upstream's direction
and then pull the fix with the next update (likey available in 2-3
months). Otherwise we have to document the changes somehow to avoid
another ksh88 nightmare incident (AFAIK you're from Sun, right ?
Remember how ksh88 in OS/Net ended-up in a completely unmaintainable
state because noone knew the patch status anymore (along with other
cool issues like disabling the testsuite and repeated hacks to satisfy
short-term goals while sacrifying long-term maintainablity) ...).

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)