rdkit-devel

[Rdkit-devel] CMake_28Sept2009 pull BLAS and LAPACK

[Riccardo V. <ric...@gm...>]

Hello,

while building rdkit from branch CMake_28Sept2009 on my linux box,
during the configuration step I noticed that blas/lapack were not
checked for (CMake behaved as if no working fortran compiler were
available, although it was).  I'm not sure I understood the
implementation in detail, but my impression is that function
workaround_9220() doesn't actually call enable_language() to set
CMAKE_Fortran_COMPILER_WORKS in the parent scope, but sets the value
of the variable it takes as second argument. It's not a big problem
and workaround_9220 will be obsoleted by a next release of cmake, but
in case you were interested the following small patch to
$RDBASE/Code/CMakeLists.txt seems to provide the expected behavior (on
my linux machine at least).

Regards,
Riccardo

$ svn diff Code/CMakeLists.txt
Index: Code/CMakeLists.txt
===================================================================
--- Code/CMakeLists.txt (revision 1231)
+++ Code/CMakeLists.txt (working copy)
@@ -25,8 +25,9 @@
 # pull BLAS and LAPACK

 # Things are easy if we have fortran:
-workaround_9220(Fortran OPTIONAL)
-IF (CMAKE_Fortran_COMPILER_WORKS)
+workaround_9220(Fortran W9220_Fortran_COMPILER_WORKS)
+IF (W9220_Fortran_COMPILER_WORKS)
+enable_language(Fortran)
 find_package(BLAS)
 find_package(LAPACK)
 ELSE()

Re: [Rdkit-devel] CMake_28Sept2009 pull BLAS and LAPACK

[Greg L. <gre...@gm...>]

Riccardo,

Thanks for the info here. I will take a look soon.

An additional piece of information: I managed over the last couple of
days to get the minimal branch building on windows (using visual
studio) with cmake. This required some changes to the CMake files. The
changes are currently only on the rdkit_minimal branch; once I'm more
comfortable that they also work smoothly on linux and the mac, these
changes will also have to be done on the rdkit_cmake branch. Maybe we
can figure out a logical division of labor for this?

-greg

On Wed, Sep 30, 2009 at 3:02 PM, Riccardo Vianello
<ric...@gm...> wrote:
> Hello,
>
> while building rdkit from branch CMake_28Sept2009 on my linux box,
> during the configuration step I noticed that blas/lapack were not
> checked for (CMake behaved as if no working fortran compiler were
> available, although it was).  I'm not sure I understood the
> implementation in detail, but my impression is that function
> workaround_9220() doesn't actually call enable_language() to set
> CMAKE_Fortran_COMPILER_WORKS in the parent scope, but sets the value
> of the variable it takes as second argument. It's not a big problem
> and workaround_9220 will be obsoleted by a next release of cmake, but
> in case you were interested the following small patch to
> $RDBASE/Code/CMakeLists.txt seems to provide the expected behavior (on
> my linux machine at least).
>
> Regards,
> Riccardo
>
> $ svn diff Code/CMakeLists.txt
> Index: Code/CMakeLists.txt
> ===================================================================
> --- Code/CMakeLists.txt (revision 1231)
> +++ Code/CMakeLists.txt (working copy)
> @@ -25,8 +25,9 @@
>  # pull BLAS and LAPACK
>
>  # Things are easy if we have fortran:
> -workaround_9220(Fortran OPTIONAL)
> -IF (CMAKE_Fortran_COMPILER_WORKS)
> +workaround_9220(Fortran W9220_Fortran_COMPILER_WORKS)
> +IF (W9220_Fortran_COMPILER_WORKS)
> +enable_language(Fortran)
>  find_package(BLAS)
>  find_package(LAPACK)
>  ELSE()
>
> ------------------------------------------------------------------------------
> Come build with us! The BlackBerry&reg; Developer Conference in SF, CA
> is the only developer event you need to attend this year. Jumpstart your
> developing skills, take BlackBerry mobile applications to market and stay
> ahead of the curve. Join us from November 9&#45;12, 2009. Register now&#33;
> http://p.sf.net/sfu/devconf
> _______________________________________________
> Rdkit-devel mailing list
> Rdk...@li...
> https://lists.sourceforge.net/lists/listinfo/rdkit-devel
>

Re: [Rdkit-devel] CMake_28Sept2009 pull BLAS and LAPACK

[Riccardo V. <ric...@gm...>]

Hi Greg,

On Wed, Sep 30, 2009 at 6:00 PM, Greg Landrum <gre...@gm...> wrote:
> [...]
> An additional piece of information: I managed over the last couple of
> days to get the minimal branch building on windows (using visual
> studio) with cmake. This required some changes to the CMake files. The
> changes are currently only on the rdkit_minimal branch; once I'm more
> comfortable that they also work smoothly on linux and the mac, these
> changes will also have to be done on the rdkit_cmake branch. Maybe we
> can figure out a logical division of labor for this?

sure. It is obviously not easy to me to plan an effective
decomposition, but once the specific requirements for the various OSs
are cleared it's likely (well, it's also my hope) that extending the
changes to the whole build could become a mostly repetitive task. In
that case I could probably try to take care of a portion of the source
tree.

Actually this reminds me of some questions I intended to ask. In
"translating" the Jamfiles into more or less corresponding cmake
listfiles, I had some troubles in understanding the pattern behind the
production of shared/dynamic vs static C++ libraries. I only read the
minimum of the boost jam docs that the task seemed to require, but -
with a few exception - it seemed that under Linux almost all libraries
were shared libraries while msvc was configured to produce static
libraries. Is this correct? I tried to clarify this myself and figure
out the possible reasons, but after a little  I gave up and decided
that asking could be wiser. As far as I know,  differently from the
default for Linux shared libraries, windows DLLs are not allowed to
have unresolved symbols, but the reason could be completely different.

The rdkit_library macro in RDKitUtils.cmake, was also added in the
hope that it could encapsulate these OS-specific library traits (and
the corresponding conditional code) in most cases.. I checked out a
fresh revision of the 'minimal' branch and I was trying to understand
if the differences are repetitive enough for this to happen, but I
have no real idea about this yet.

-- 
Ric

Re: [Rdkit-devel] CMake_28Sept2009 pull BLAS and LAPACK

[Greg L. <gre...@gm...>]

Ric,

On Wed, Sep 30, 2009 at 11:38 PM, Riccardo Vianello
<ric...@gm...> wrote:
>
> On Wed, Sep 30, 2009 at 6:00 PM, Greg Landrum <gre...@gm...> wrote:
>> [...]
>> An additional piece of information: I managed over the last couple of
>> days to get the minimal branch building on windows (using visual
>> studio) with cmake. This required some changes to the CMake files. The
>> changes are currently only on the rdkit_minimal branch; once I'm more
>> comfortable that they also work smoothly on linux and the mac, these
>> changes will also have to be done on the rdkit_cmake branch. Maybe we
>> can figure out a logical division of labor for this?
>
> sure. It is obviously not easy to me to plan an effective
> decomposition, but once the specific requirements for the various OSs
> are cleared it's likely (well, it's also my hope) that extending the
> changes to the whole build could become a mostly repetitive task. In
> that case I could probably try to take care of a portion of the source
> tree.

I'm going to take a look this morning to see how much of the required
changes can be abstracted out and put into a central location (as you
did already with a lot of the library stuff).

>
> Actually this reminds me of some questions I intended to ask. In
> "translating" the Jamfiles into more or less corresponding cmake
> listfiles, I had some troubles in understanding the pattern behind the
> production of shared/dynamic vs static C++ libraries. I only read the
> minimum of the boost jam docs that the task seemed to require, but -
> with a few exception - it seemed that under Linux almost all libraries
> were shared libraries while msvc was configured to produce static
> libraries. Is this correct? I tried to clarify this myself and figure
> out the possible reasons, but after a little  I gave up and decided
> that asking could be wiser. As far as I know,  differently from the
> default for Linux shared libraries, windows DLLs are not allowed to
> have unresolved symbols, but the reason could be completely different.

It is, unfortunately, a bit more complex than this. A bit of explanation:
I'd prefer to have everything:
1) consistent (to make it easier to get your brain around things)
2) as shared libraries (so updating is easier, among other reasons)
When I did the Jamfiles, getting the dependencies right on Windows so
that everything worked as a DLL turned out to be beyond my
patience/skills, so I did most everything as static libraries. On
linux (at least, this might also be true for the Mac), some of the
libraries *need* to be dynamic in order for the python wrappers to
work correctly (has to do with passing objects between different
python modules). So we're kind of stuck with some stuff needing to be
static and some needing to be dynamic. This lead to the situation you
now see in the Jamfile-based build: dynamic libraries on linux and the
Mac and static libraries on windows.

> The rdkit_library macro in RDKitUtils.cmake, was also added in the
> hope that it could encapsulate these OS-specific library traits (and
> the corresponding conditional code) in most cases.. I checked out a
> fresh revision of the 'minimal' branch and I was trying to understand
> if the differences are repetitive enough for this to happen, but I
> have no real idea about this yet.

I believe this should be mostly do-able. As I said, I'll take a look
this morning once I've more thoroughly convinced myself that things
are actually working on the minimal branch.

-greg

Re: [Rdkit-devel] CMake_28Sept2009 pull BLAS and LAPACK

[Greg L. <gre...@gm...>]

Riccardo,

Well, it took longer than I hoped, but it looks like things now more
or less work on windows, linux, and the Mac (rev1237).

I had to make a fair number of modifications due to the static linking
on windows, and it might be worth going back at some point and
cleaning things up, but what's there now seems to be functional.

Let me know what you think.

-greg

Re: [Rdkit-devel] CMake_28Sept2009 pull BLAS and LAPACK

[Riccardo V. <ric...@gm...>]

Hi,

> I had to make a fair number of modifications due to the static linking
> on windows, and it might be worth going back at some point and
> cleaning things up, but what's there now seems to be functional.
>
> Let me know what you think.

it's been a busy day and unfortunately I only had time for a quick
tour.. in browsing a sample of the involved files, I had the
impression that modifying almost every cmake listfile was most often
required by making link-time dependencies explicit again, is that
correct? There are of course other changes here and there, but they
seem more localized and involve a more limited number of files...

I hope to be able to provide a better feedback tomorrow..

-- 
Ric

Re: [Rdkit-devel] CMake_28Sept2009 pull BLAS and LAPACK

[Greg L. <gre...@gm...>]

On Fri, Oct 2, 2009 at 1:05 AM, Riccardo Vianello
<ric...@gm...> wrote:
>
>> I had to make a fair number of modifications due to the static linking
>> on windows, and it might be worth going back at some point and
>> cleaning things up, but what's there now seems to be functional.
>>
>> Let me know what you think.
>
> it's been a busy day and unfortunately I only had time for a quick
> tour.. in browsing a sample of the involved files, I had the
> impression that modifying almost every cmake listfile was most often
> required by making link-time dependencies explicit again, is that
> correct? There are of course other changes here and there, but they
> seem more localized and involve a more limited number of files...

Yes, the changes are almost all adding libraries that the various
regressions and python modules need to be linked against. The changes
end up being massively redundant, since for example linking against
GraphMol also requires RDGeneral and RDGeometryLib, but the
information does need to be provided.

I guess one possibility to make life easier would be to modify
rdkit_library so that it defines a new variable containing the new
library name along with that libraries dependencies and then using
those new variables for regressions. So something like:

rdkit_library(GraphMol
              Atom.cpp <clipped for clarity> Conformer.cpp
              LINK_LIBRARIES RDGeometryLib RDGeneral )

would define the new variable GraphMol_LIBS (or some such thing) that
could be used to replace:

rdkit_test(graphmolTest1 test1.cpp LINK_LIBRARIES GraphMol
RDGeometryLib RDGeneral)

with:

rdkit_test(graphmolTest1 test1.cpp LINK_LIBRARIES GraphMol_LIBS)

This would probably lead to a lot of redundancy for cmake to deal
with, since something like:

rdkit_test(graphmoltestCanon testCanon.cpp
           LINK_LIBRARIES SubstructMatch_LIBS SmilesParse_LIBS GraphMol_LIBS)

is going to expand to:

rdkit_test(graphmoltestCanon testCanon.cpp
           LINK_LIBRARIES SubstructMatch GraphMol RDGeometryLib
RDGeneral SmilesParse GraphMol RDGeometryLib RDGeneral  GraphMol
GraphMol RDGeometryLib RDGeneral )

but I would hope that cmake could deal with that.

What do you think?

-greg



>
> I hope to be able to provide a better feedback tomorrow..
>
> --
> Ric
>