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

[Garrett D'Amore]

On Fri, 2011-01-21 at 00:19 +0100, Roland Mainz wrote:
> 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.

Right... if we want a better regex, then lets do it in our libc, not
bolt on some other library.  And *not* as part of this change.

> 
> > 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)).

This 64 bit mission you seem to be on is kind of irrelevant here.

As long as the application works fine in 32-bit mode (large file
support) there is little point in compiling it for 64-bit mode.

That said, split architecture directories is probably a good thing in
that it leads longer term towards the idea that we could have one
compile tree that could support simultaneous compilation of both sparc
and x86.  (But that's a ways away.)

> 
> [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).

Um, no.  Wherever you got that idea, its wrong.

The utilities in XPG4/6 must be compliant with those standards.  That
may require them to use libraries, which require these flags. 

> 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
> 

The system() call may be an example.  Looking at it, it looks like the
only difference is whether /usr/xpg4/bin/sh or "sh" is called.  We are
fast approaching the point where that doesn't matter, I think.
(When /bin/sh is just ksh93... it won't matter.)


> > 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) ...).

Please stop with the ksh88 history.  I refuse to accept that the only
way we can update our code is to put changes upstream, and wait for them
to come back downstream in a few months.  I *do* think we should offer
our changes back upstream, but I don't think we should necessarily hold
ourselves hostage to whatever is the case in the upstream.

In any event, I think we have at least one real bug to fix, which is the
default handling when called as "more", or -e is issued.  This is the
bug Joerg talked about.  We need a fix for that, and we need it before
it comes back down from upstream (although we should offer the fix to
the upstream as well.)

	- Garrett