Added customization for make targets in 'build' and 'install' phases for CMakePackage #2742

adamjstewart · 2017-01-04T21:37:47Z

Does the exact same thing as #2464 but for CMakePackage.

In #2602, @fedepad asked how to run custom build targets in CMakePackage. This PR adds that functionality. For espressopp, I believe this would look like:

build_targets = ['all', 'docs']

adamjstewart · 2017-01-04T22:32:36Z

Also allows AutotoolsPackages to be built in a different directory. Some packages like mozjs require this. All 3 build systems should be much more uniform now.

alalazo

I'll have time later in the day to try this PR, but at a first glance I agree with all the changes.

adamjstewart · 2017-01-05T17:24:26Z

Things may need to be made a little more robust. There are two situations I envision:

configure exists in the root directory, but must be run from a different directory
configure exists in a sub-directory, like src

We should make sure that both work.

alalazo · 2017-01-05T17:31:29Z

@adamjstewart Do you know any relevant package that does 2? Because from this to have multiple configure in the same tarball the step is tiny...

adamjstewart · 2017-01-05T17:42:16Z

tk is a good example of 2. The configure script is located in the unix directory.

adamjstewart · 2017-01-08T18:48:59Z

@alalazo I got the tk/tcl example working. Just had to do some minor tweaks. Confirmed that "normal" packages like zlib still work. All changes are attached to this PR so you can see what it would look like for other packages.

…for CMakePackage

adamjstewart · 2017-01-08T19:46:34Z

This introduces a strange new bug. When I configure m4, it creates a GNUMakefile file, which is a symlink to itself. make calls this file:

==> 'make' '-j4'
make: GNUmakefile: Too many levels of symbolic links
make: stat: GNUmakefile: Too many levels of symbolic links
make: *** No rule to make target `GNUmakefile'.  Stop.

Looks like it was something in the 782d8b9 commit that caused this change. The only difference in the log file is that it now runs /absolute/path/to/configure instead of ./configure, but I can't reproduce the bug by doing that outside of Spack.

EDIT: I think the problem is that the build directory is a symlink, and it's trying to create a symlink from there, and it's running into an infinite loop of recursion. No idea how to prevent that. I'm surprised my change caused such a serious problem.

EDIT: According to http://gumstix.8.x6.nabble.com/GNUmakefile-too-many-levels-symbolic-links-td665281.html, the problem may be because we are building in a symlinked directory:

Is your tmp directory a symbolic link or have you manually created any symbolic links within tmp? Regrettably, it is not possible to have a symbolically linked tmp directory; this is a known bug with the OpenEmbedded build system.

…ory than build_directory

adamjstewart · 2017-01-09T01:58:25Z

Ok, I backed out the changes that allowed you to run a configure script in a different directory. I don't think absolute paths will work if the directory is symlinked, so we could possibly get away with relative paths, but I decided we don't need it enough right now.

Confirmed that tcl and tk build with these changes. Building espressopp now.

adamjstewart · 2017-01-09T13:11:50Z

Tried building espressopp +ug +pdf +dg but it crashed with the following error message:

Running Sphinx v1.4.5
making output directory...
Warning: numpy module not available
WARNING: sphinx.ext.pngmath has been deprecated. Please use sphinx.ext.imgmath instead.

Extension error:
Could not import extension matplotlib.sphinxext.only_directives (exception: No module named matplotlib.sphinxext.only_directives)

I don't think this is related to my changes. @fedepad @junghans Any idea what is causing this to fail?

fedepad · 2017-01-09T14:42:18Z

@adamjstewart but the package itself is compiled ok (no doc)? Since what you report regards building of the doc, in particular the user doc. I never seen this on my machine but this sphinx-doc/sphinx#1684 seems to mention matplotlib should be installed since one of sphinx extension is using it...in this case we should add matplotlib as a dependencies when building user doc... @junghans Ideas? We could try that...

fedepad · 2017-01-09T14:52:43Z

@adamjstewart would you try adding the following lines:
depends_on('py-matplotlib', when="+ug")
depends_on('py-matplotlib', when="+pdf")
and see what happens? I'm not sure about the type of this dependency and not sure we need the second line I wrote.

fedepad · 2017-01-09T14:58:08Z

@junghans and because of the warning on numpy I wonder whether we need numpy too...

adamjstewart · 2017-01-09T15:24:40Z

I'll try that now. Btw, the changes I made require me to run the entire installation in serial, not just the documentation building steps. I'm not sure if that makes this change worth it for this particular package. Thoughts?

junghans · 2017-01-09T15:34:00Z

Yes, numpy is used in the tutorial and src/external in the docker container, we install it, too:
https://github.com/espressopp/buildenv/blob/master/ubuntu#L3
to build the documentation.

citibeth · 2017-01-09T15:39:13Z

No not worth it. Serial builds are maddeningly slow and take a noticeably disproportionate fraction of the build time.

…

On Jan 9, 2017 10:24 AM, "Adam J. Stewart" ***@***.***> wrote: I'll try that now. Btw, the changes I made require me to run the entire installation in serial, not just the documentation building steps. I'm not sure if that makes this change worth it for this particular package. Thoughts? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#2742 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cdx1EO5hikzBJ2bMmcLLa_u7WEyPnks5rQlE5gaJpZM4LbGCr> .

fedepad · 2017-01-09T15:39:33Z

@adamjstewart I've experienced that building espressopp is usually kind of slow so I usually turn on a parallel build, which I highly suggest, so I'm not sure if it makes sense in this case, but maybe @junghans had other experience...

@junghans Ok, then we need to update and add those dependencies I mentioned for building the docs ;)

junghans · 2017-01-09T15:43:36Z

@fedepad: Yeah, parallel builds work in espresso++. Shouldn't spack build everything in parallel by default?

adamjstewart · 2017-01-09T15:47:58Z

Shouldn't spack build everything in parallel by default?

Yes, but my changes replaced your build override with a list of build targets. Since I could no longer specify parallel=False per-build, I had to specify it for the entire package.

I'll back out that particular change. I agree that building in parallel is way faster. I was able to confirm that my new build_targets list does work as expected though, so that's good.

fedepad · 2017-01-09T15:48:16Z

@junghans you can configure it, whether parallel or serial. What @adamjstewart was referring to is that bringing the changes proposed in this PR to Espresso++ (with the modification proposed in one of the commits) will require to run a serial build. Did I understood correctly?

adamjstewart · 2017-01-09T16:09:16Z

Ok, with the latest commit, the package now builds in parallel. I added the numpy and matplotlib dependencies, and it gets farther in the documentation building steps, but now crashes with:

! Package keyval Error: pdfencoding undefined.

See the keyval package documentation for explanation.
Type  H <return>  for immediate help.
 ...                                              
                                                  
l.2238 \ProcessKeyvalOptions{Hyp}

Must be some LaTeX package that's missing as a dependency.

I think this PR is safe to merge now. I'll let @fedepad and @junghans work out the details of the documentation builds in another PR.

fedepad · 2017-01-09T17:23:08Z

@adamjstewart @junghans I might have an idea of what's going on. It might be doxygen using the new key pdfencoding in the hyperref latex package. So a probable solution might be to have an updated texlive and requiring the live version (the only one in spack).
@adamjstewart Can you tell me if you ran it on your local machine and which version of texlive and doxygen you had? Highly appreciated, thanks!

adamjstewart · 2017-01-09T17:30:26Z

So a probable solution might be to have an updated texlive and requiring the live version (the only one in spack).

Aren't texlive and doxygen already dependencies of espressopp? Shouldn't it be picking them up from the PATH?

@adamjstewart Can you tell me if you ran it on your local machine and which version of texlive and doxygen you had?

I ran this on our cluster.

$ tex --version
TeX 3.141592 (Web2C 7.5.6)
kpathsea version 3.5.6
Copyright 2007 D.E. Knuth.
Kpathsea is copyright 2007 Karl Berry and Olaf Weber.
There is NO warranty.  Redistribution of this software is
covered by the terms of both the TeX copyright and
the Lesser GNU General Public License.
For more information about these matters, see the file
named COPYING and the TeX source.
Primary author of TeX: D.E. Knuth.
Kpathsea written by Karl Berry, Olaf Weber, and others.

$ doxygen --version
1.6.1

adamjstewart · 2017-01-09T22:47:54Z

Ping @tgamblin

alalazo · 2017-01-10T08:39:33Z

lib/spack/spack/build_systems/cmake.py

        """
        return []

    def cmake(self, spec, prefix):
        """Run cmake in the build directory"""
        options = [self.root_cmakelists_dir()] + self.std_cmake_args + \
            self.cmake_args()
-        create = not os.path.exists(self.build_directory())
-        with working_dir(self.build_directory(), create=create):
+        with working_dir(self.build_directory(), create=True):


@adamjstewart Will this work in case the build directory already exists?

Yes I believe so. I'll have to check how we define working_dir, but if I remember correctly it only creates the directory if it doesn't exist.

Looks like it calls mkdirp, which does indeed check to see whether or not the directory exists first, so we can set create=True and not worry.

adamjstewart · 2017-01-15T13:43:22Z

Ping ping @tgamblin

adamjstewart added the cmake label Jan 4, 2017

tgamblin added the build-environment label Jan 4, 2017

adamjstewart requested a review from alalazo January 4, 2017 22:32

fedepad mentioned this pull request Jan 4, 2017

espressopp: package for the ESPResSo++ software #2602

Merged

citibeth approved these changes Jan 5, 2017

View reviewed changes

alalazo approved these changes Jan 5, 2017

View reviewed changes

adamjstewart added 6 commits January 8, 2017 12:49

Added customization for make targets in 'build' and 'install' phases …

4e6288e

…for CMakePackage

Use rst in build system docs so that Sphinx generates nice API docs

d6d6e2e

Allow AutotoolsPackages to be built in a different directory

8a0a9b3

Flake8

9e579b3

Fix missing import

9c239ae

Allow configure to be located in different directory

782d8b9

adamjstewart force-pushed the features/cmake-build-targets branch from 2add86d to 782d8b9 Compare January 8, 2017 18:49

Update espressopp to use build targets

1175fc6

Flake8

a4fdf38

adamjstewart changed the title ~~Added customization for make targets in 'build' and 'install' phases for CMakePackage~~ [WIP] Added customization for make targets in 'build' and 'install' phases for CMakePackage Jan 8, 2017

adamjstewart added the WIP label Jan 8, 2017

adamjstewart added 2 commits January 8, 2017 17:05

Sphinx fix, lists must be a new paragraph

e9e80a4

Back out change that allowed a configure script in a different direct…

f4c5c97

…ory than build_directory

adamjstewart removed the WIP label Jan 9, 2017

adamjstewart changed the title ~~[WIP] Added customization for make targets in 'build' and 'install' phases for CMakePackage~~ Added customization for make targets in 'build' and 'install' phases for CMakePackage Jan 9, 2017

adamjstewart mentioned this pull request Jan 9, 2017

documentation: build-system phases + build-time tests #2780

Merged

2 tasks

Add missing deps, build in parallel

bf200c6

alalazo reviewed Jan 10, 2017

View reviewed changes

Missing space for rst list

5487857

tgamblin approved these changes Jan 16, 2017

View reviewed changes

tgamblin merged commit f480e34 into spack:develop Jan 16, 2017

This was referenced Jan 17, 2017

Building an Autotools package in a different directory #2428

Closed

AutotoolsPackage: minor improvements #2859

Merged

adamjstewart deleted the features/cmake-build-targets branch January 20, 2017 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added customization for make targets in 'build' and 'install' phases for CMakePackage #2742

Added customization for make targets in 'build' and 'install' phases for CMakePackage #2742

adamjstewart commented Jan 4, 2017

adamjstewart commented Jan 4, 2017 •

edited

alalazo left a comment

adamjstewart commented Jan 5, 2017

alalazo commented Jan 5, 2017

adamjstewart commented Jan 5, 2017

adamjstewart commented Jan 8, 2017

adamjstewart commented Jan 8, 2017 •

edited

adamjstewart commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

fedepad commented Jan 9, 2017

fedepad commented Jan 9, 2017 •

edited

fedepad commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

junghans commented Jan 9, 2017

citibeth commented Jan 9, 2017 via email

fedepad commented Jan 9, 2017

junghans commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

fedepad commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

fedepad commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

alalazo Jan 10, 2017

adamjstewart Jan 10, 2017

adamjstewart Jan 10, 2017

adamjstewart commented Jan 15, 2017

Added customization for make targets in 'build' and 'install' phases for CMakePackage #2742

Added customization for make targets in 'build' and 'install' phases for CMakePackage #2742

Conversation

adamjstewart commented Jan 4, 2017

adamjstewart commented Jan 4, 2017 • edited

alalazo left a comment

Choose a reason for hiding this comment

adamjstewart commented Jan 5, 2017

alalazo commented Jan 5, 2017

adamjstewart commented Jan 5, 2017

adamjstewart commented Jan 8, 2017

adamjstewart commented Jan 8, 2017 • edited

adamjstewart commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

fedepad commented Jan 9, 2017

fedepad commented Jan 9, 2017 • edited

fedepad commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

junghans commented Jan 9, 2017

citibeth commented Jan 9, 2017 via email

fedepad commented Jan 9, 2017

junghans commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

fedepad commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

fedepad commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

adamjstewart commented Jan 9, 2017

alalazo Jan 10, 2017

Choose a reason for hiding this comment

adamjstewart Jan 10, 2017

Choose a reason for hiding this comment

adamjstewart Jan 10, 2017

Choose a reason for hiding this comment

adamjstewart commented Jan 15, 2017

adamjstewart commented Jan 4, 2017 •

edited

adamjstewart commented Jan 8, 2017 •

edited

fedepad commented Jan 9, 2017 •

edited