Thanks Mark,
Responses are inline.
Rod, I've cc'd you, because there are things (referenced below)
that I'd appreciate your looking at.
Mark J. Nelson wrote:
>
>> I have written a heads up message that describes all of this in more
>> detail:
>>
>> http://cr.opensolaris.org/~alib/mapfilechk.heads_up
>
> "...ask for help, and from who" should read "...ask for help, and from
> whom."
Yes, fixed
>
> It's probably not a big deal, but the first sentence under "2)" gives
> the impression that mapfilechk, like hg mapfilechk, is subject to
> *mapfile* regex filtering.
I rewrote it:
2) There is a new Mercurial/Cadmium command, 'hg mapfilechk',
that is run against any file in ON matching the pattern
'*mapfile*'. This tool operates much as cddlchk does, and
in fact shares a common implementation under the covers.
mapfilechk ensures that the comments described in (1) are
not altered or removed, and that new mapfiles introduced
into ON do not integrate without one.
There is also a command line version of mapfilechk, found in
usr/src/tools/scripts, that will examine any files you point
it at. This works similarly to the cddlchk command found in
the same directory.
mapfilechk is included by 'hg nits' and 'hg pbchk'.
>
> For "mapfiles that we know do not (and will not) specify symbol
> versioning," would it actually be inappropriate to embed the comment,
> instead of excluding them?
No, and in fact, my feeling is that it is better to put the comment in
a mapfile that does not need it, than it is to ensure that we only target
mapfiles that do need it. I'm intentionally painting with a broad brush.
Note that I'm concerned not only with todays set of mapfiles, but also with:
- New ones that might be added in the future (and which might not
use the recommended mapfile-vers name).
- Existing ones that don't do versioning today, but which might be
changed to in the future.
This is a heuristic business. I'm willing to hit a few that don't need the
comment, in order to lower the odds that I ever miss one that does. This means
that in a few relatively rare cases, an experienced person may read a mapfile
and say "that comment isn't needed". I'm OK with that. If a particular mapfile
proves to be confusing in the future, we can exclude it then.
However: Rod pointed out that even by that standard, there are some mapfiles
that really have nothing to do with versioning, and that one can confidently say
never will. I did exclude those --- there's no point in being *too* rigid...
:-)
It turns out to be ~12 files out of ~525.
>
>> Webrev for the change is available on SWAN at:
>>
>> http://linkers/webrev/6785284/index.html
>>
>> or externally:
>>
>> http://cr.opensolaris.org/~alib/mapfilechk/
>
> Your CRT advocate will appreciate it if you update the bug reports
> before filing your RTI. The defects (as opposed to RFE) will need
> introduced in/root cause info, and all will need suggested fixes.
Yes. I left this for post code review, since I expected to make some changes
before integration.
>> There are 515 files involved in this, almost all of which are
>> mapfiles that received the comment, and possibly had their copyright
>> or cddl blocks updated. I certainly don't expect (or need) many of you
>> to wade through all of these (although I have looked at every one, and
>> Rod
>> has already code reviewed this for me). I'd say that if you're interested
>> at all, just hitting a few randomly would suffice.
>>
>> Here is a list of the files that are not mapfiles, that might merit
>> your attention, based on your interest:
>
> I rearranged the list a little bit, in an attempt to make it clear what
> I had/had not reviewed. Let me know if I didn't succeed.
>
>> exception_lists/mapfilechk
>
> I didn't expect questions here. :) Nonetheless: why a glob for
> usr/src/tools/scripts, when all the rest are so much more
> filename-specific?
There are two files there, mapfilechk.py, and mapfilechk.1. I could
have given them separate lines (originally did), or used 'mapfilechk*'.
However, I reasoned] that a *scripts* directory will never contain a
linker mapfile, and used the glob. I'll change it if you object.
>
>> usr/src/tools/onbld/Checks/CmtBlk.py
>
> ...seems to be missing a word in line 30:
> 30 # Check source files contain a valid comment block
A faithful cut/paste from the original Cddl.py:
# Check source files contain a valid CDDL block
I've fixed them both.
>
>> usr/src/tools/scripts/mapfilechk.1
>
> Do you have this installed on a system that I can just check the manpage?
rtld.central
>
>> usr/src/tools/onbld/Checks/Mapfile.py
>> usr/src/tools/scripts/mapfilechk.py
>> usr/src/tools/onbld/Checks/Cddl.py
>
> To be perfectly honest, I assume that this was a complete cut/paste job.
> I didn't carefully review the code that you moved from one place to
> another. With that caveat, these looked like I expected.
Indeed, I was gratified to be able to leverage the existing stuff.
However, it's not entirely cut/paste, because I didn't make 2 copies
of the core Cddl.py ---- I factored it into a form where both cddlchk
and mapfilechk can use it.
>> usr/src/lib/README.mapfiles
>
> Comment on 207-213:
> - s/the OSnet/the OSNet consolidation/
> - s/SUNWpriviate/SUNWprivate/
Fixed.
> - I'm not actually sure that this paragraph provides useful
> clarification? And what IS the best practice here? Isn't it to use
> SUNWprivate for consolidation- or project-private interfaces? Ie
> something that's contracted should be in a public version, shouldn't it?
Yes, something that's contracted should be in a public version.
libld.so contains only private interfaces. Still, the entire linker subsystem,
which we build and deliver as a single unit, depends on that interface. We
increment the private versions on libld.so in order to catch those *clever*
folks who think they can copy libld.so from some other system and drop it
into place. Presumably, this has happened in the past, spurring Rod and Mike
to take this step years ago. I've found it useful in my work...
Given that there's at least one mapfile in ON that does this, I felt the
possibility should be captured in the document.
The best practice for most private libraries is not to add the numbers,
and README.mapfiles does say this. This new paragraph just says "but if you
encounter a mapfile that does this, respect the local convention".
>
> Comment on 339-347:
> - s/Simply remove/To move an interface from Private to local scope,
> simply remove/
That's better
> - The last sentence here seems like a separate paragraph, or even a
> separate, numbered section.
Although it could be, I think it's OK where it is.
>
>> usr/src/tools/SUNWonbld/prototype_com
>> usr/src/tools/onbld/Checks/Makefile
>> usr/src/tools/onbld/Checks/__init__.py
>> usr/src/tools/onbld/hgext/cdm.py
>> usr/src/tools/scripts/Makefile
>> usr/src/tools/scripts/check_rtime.1
>
> These all look OK.
>
>> usr/src/tools/scripts/check_rtime.pl
>
> I'm NOT a Perl guy, I would prefer somebody else reviewed these changes.
I'll have Rod look it over again, and Jim has also examined it in his comments.
>
>> usr/src/tools/scripts/nightly.sh
>
> - Do we still care about people running our nightly script on
> pre-Solaris-9 systems? Maybe, but "ugh." (ref. lines 2959-2965)
No, we don't. I'll consult with Rod and see.
> - While John Forte was merging NWS bits, I spent a *ton* of time helping
> him figure out a Makefile problem that was giving him a 32-bit object
> when it should have been 64-bit. Is the LDDWRONG string (ref. line
> 2966) actually correct? Because that would have gotten us there MUCH
> sooner, I suspect. (We fell through this test, but then still hit the
> next one, about CRLECONF.)
I'm not sure, but I think it covers the case where you have a 32-bit kernel
that can't run the 64-bit ldd. I'll ask Rod about that too...
I'll refresh the webrev after I work through Jim's comments...
- Ali
