[cmake-developers] Reworking SystemInformation

David Cole david.cole at kitware.com
Sat Oct 1 12:56:14 EDT 2011


I think it would be good to do what you propose, eventually. Emphasis on the
eventually, unless there's a burning need for it on the part of a
contributor.

However, the KWSys files are not directly push-able into our git repository:
we have a server-side hook that rejects pushes containing changes to KWSys
files. This makes accepting patches for it a bit more work for us than a
simple "git am -3" -- the real KWSys files are still in a shared CVS
repository, and we mirror them into our git repo through a robot. So... we
have to apply your patch to the CVS repo, and wait for the mirror to replay
them into the CMake git repo.

Furthermore, you cannot simply analyze CMake and say that some KWSys
functionality is definitively "unused" -- KWSys is shared among many, many
projects, (including VTK, ITK, gccxml, ParaView, and others...) any of which
may actually be using something in it that you propose to remove.

Personally, I hate much of the code that is in SystemInformation.cxx, and I
try to avoid using it or relying on it for anything critically
cross-platform. As you've observed, there are several holes in it that are
in need of plugging to make it robust and useful/use-able...

Looks like your patch was based on some other commit that is not in 'master'
...? I get this when I try to apply it:

davidcole at HUT11 ~/Dashboards/My Tests/CMake (sysinfo-rework)
$ git am -3 ~/Downloads/0001-remove-trailing-whitespace.patch
Applying: remove trailing whitespace
fatal: sha1 information is lacking or useless
(Source/kwsys/SystemInformation.cxx).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 remove trailing whitespace
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".


I'm willing to remove things that are truly unused, and cluttering up the
interface. But first we have to be absolutely sure they are unused across
all KWSys clients. And I'm more inclined to add to the interface to return a
string for a CPU identifier than to *change* the existing interface. I'd
rather leave well enough alone as much as possible in KWSys, and add new
stuff that is fully cross-platform as needed moving forward.


HTH,
David


On Sat, Nov 6, 2010 at 3:31 PM, Rolf Eike Beer <eike at sf-mail.de> wrote:

> I wanted to have a look if I could fix one of my bugs (0010895: Processor
> detection is very x86-specific). What I found was some things in kwsys
> classes
> I'm not sure about.
>
> First there is a bunch of files that have trailing whitespaces over and
> over
> which seems rather randomly scattered over the files. I did
>
>  for i in *; do sed 's/[ \t]*$//' -i $i; done
>
> which lead to the attached patch (0001).
>
> Next I found a thing called testSystemInformation.cxx. Looks like it is a
> program to debug the SystemInformation class. It is the only user of some
> things like DoesCPUSupportCPUID(), GetProcessorAPICID(), and
> DoesCPUSupportFeature() from that class. What about just kicking them out?
> AFAICT this is Windows-only stuff so this is pretty useless on nearly all
> architectures anyway. I just deleted some of them and it still compiles
> without problems so I don't see any problems (patch 0002).
>
> The next thing is the root cause of the above bug: nearly everything in
> that
> class is centered around Intel like CPUs. While that is indeed the majority
> of
> the platforms that use CMake (at least in numbers of users) this makes
> support
> for other CPUs a bit harder. E.g. storing the processor family as integer
> makes sense on x86 as it's encoded this way there but what /proc/cpuinfo on
> my
> C3600 prints as family id is "PA-RISC 2.0" which doesn't fit in an integer
> at
> all. I would like to rework that so the transition from integer to string
> is
> done immediately on query and the string is stored also on x86 so non-x86
> would work without any further action at least on Linux.
>
> Comments?
>
> Eike
>
> --
>
> Powered by www.kitware.com
>
> Visit other Kitware open-source projects at
> http://www.kitware.com/opensource/opensource.html
>
> Please keep messages on-topic and check the CMake FAQ at:
> http://www.cmake.org/Wiki/CMake_FAQ
>
> Follow this link to subscribe/unsubscribe:
> http://public.kitware.com/cgi-bin/mailman/listinfo/cmake-developers
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20111001/bcdbd700/attachment.html>


More information about the cmake-developers mailing list