Heh. The list overrides the reply-to. :-)
Agreed -- let's you, me, and Brian discuss off-list and let anyone who
cares know what the final solution is that we come up with.
FWIW, we've become quite fond of developing in mercurial branches and
pushing them somewhere to share when you want to get others opinions
before committing to the trunk. bitbucket.org, for example, offers
free mercurial hosting. I use it quite heavily.
On Jul 22, 2009, at 1:46 PM, Iain Bason wrote:
On Jul 22, 2009, at 10:55 AM, Brian W. Barrett wrote:
> The current autodetect implementation seems like the wrong approach
> to me. I'm rather unhappy the base functionality was hacked up like
> it was without any advanced notice or questions about original
> design intent. We seem to have a set of base functions which are now
> more unreadable than before, overly complex, and which leak memory.
First, I should apologize for my procedural misstep. I had asked my
group here at Sun whether I should do an RFC or something, and I guess
I didn't make my question clear enough. I was under the impression
that I should check something in and let people comment on it.
That being said, I would argue that there are good reasons for adding
some complexity to the base component:
1. The pre-existing implementation of expansion is broken (although
the cases for which it is broken are somewhat obscure).
2. The autodetect component cannot set more than one directory without
some knowledge of the relationships amongst the various directories.
Giving it that knowledge would violate the independence of the
components.
You can see #1 by doing "OPAL_PREFIX='${datarootdir}/..'
OPAL_DATAROOTDIR='/opt/SUNWhpc/HPC8.2/share' mpirun hostname" (if you
have installed in /opt/SUNWhpc/HPC8.2). Yes, I know, "Why would
anyone do that?" Nonetheless, I find that a poor excuse for having a
bug in the code.
To expand on #2 a little, consider the case where OpenMPI is
configured with "--prefix=/path/one --libdir=/path/two". We can tell
that libopen-pal is in /path/two, but it is not correct to assume that
the prefix is therefore /path. (Unfortunately, there is code in
OpenMPI that does make that sort of assumption -- see orterun.c.) We
need information from the config component to avoid making incorrect
assumptions.
There are, of course, alternate ways of getting to the same point.
But it is not feasible simply to leave the design of the base
component unchanged. (More on that below.)
As for readability, I am always open to constructive suggestions as to
how to make code more readable. I didn't fix the memory leak because
I hadn't yet found a way to do that without decreasing readability.
> The intent of the installdirs framework was to allow this type of
> behavior, but without rehacking all this infer crap into base. The
> autodetect component should just set $prefix in the set of functions
> it returns (and possibly libdir and bindir if you really want, which
> might actually make sense if you guess wrong), and let the expansion
> code take over from there. The general thought on how this would
> work went something like:
>
> - Run after config
> - If determine you have a special $prefix, set the
> opal_instal_dirs.prefix to NULL (yes, it's a bit of a hack) and
> set your special prefix.
> - Same with bindir and libdir if needed
> - Let expansion (which runs after all components have had the
> chance to fill in their fields) expand out with your special
> data
If we run the autodetect component after config, and allow it to
override values that are already in opal_install_dirs, then there will
be no way for users to have environment variables take precedence.
(Let's say someone runs with OPAL_LIBDIR=/foo. The autodetect
component will not know whether opal_install_dirs.libdir has been set
by the env component or by the config component.)
Moreover, if the user has used an environment variable to override one
of the paths in the config component, then the autodetect component
may make the wrong inference. For example, let's say someone runs
with OPAL_LIBDIR=/foo. The autodetect component finds libopen-pal
in /
usr/renamed/lib, and sets opal_install_dirs.libdir to /usr/renamed/
lib. However, it has to use the config component's idea of libdir
(e.g., ${exec_prefix}/lib) to correctly infer that prefix should be /
usr/renamed. Since it only has /foo from the environment variable, it
will have to decide that it cannot infer the prefix.
All of this will lead to behavior that users will have trouble
diagnosing. While I appreciate simple code, I think that a simple
user interface is more important.
We could add some infrastructure so that the autodetect component can
figure out the provenance of each field in opal_install_dirs, but that
would make the boundary between the base component and the autodetect
component unclear.
> And the base stays simple, the components do all the heavy lifting,
> and life is happy.
Except in the cases where it doesn't work.
> I would not be opposed to putting in a "find expaneded part" type
> function that takes two strings like "${prefix}/lib" and "/usr/
local/
> lib" and returns "/usr/local" being added to the base so that other
> autodetect-style components don't need to handle such a case, but
> that's about the extent of the base changes I think are appropriate.
>
> Finally, a first quick code review reveals a couple of problems:
>
> - We don't AC_SUBST variables adding .lo files to build sources in
> OMPI. Instead, we use AM_CONDITIONALS to add sources as needed.
> - Obviously, there's a problem with the _happy variable name
> consistency in configure.m4
> - There's a naming convention issue - files should all start with
> opal_installdirs_autodetect_, and a number of the added files
> do not.
Thanks. I will fix those.
> - From a finding code standpoint, I'd rather walkcontext.c and
> backtrace.c be one file with #ifs - for such short functions,
> it makes it more obvious that it's just portability
implementations
> of the same function.
I'm afraid we'll just have to disagree on the aesthetics of this. I
find that #ifs almost always make code harder to follow.
However, I'm willing to hold my nose if necessary.
> I'd be happy to discuss the issues further or review any code before
> it gets committed. But I think the changes as they exist today
> (even with bugs fixed) aren't consistent with what the installdirs
> framework was trying to accomplish and should be reworked.
Let's discuss this off-alias. I have set the reply-to field
accordingly.
Iain
_______________________________________________
devel mailing list
de...@open-mpi.org
http://www.open-mpi.org/mailman/listinfo.cgi/devel
--
Jeff Squyres
jsquy...@cisco.com
