AutotoolsPackage: minor improvements #2859

alalazo · 2017-01-18T13:33:13Z

Modifications:

added the method configure_directory to permit build out of source
configure script executable is now invoked with an absolute path
default behavior for the autoreconf phase
modified a few packages accordingly

alalazo · 2017-01-18T13:43:17Z

There's another thing that I could do either here or in a separate PR: turn build_directory & Co. into properties in all the build_systems modules.

If we do that we may write something like:

build_directory = 'whatever'

instead of:

def build_directory(self):
    return 'whatever'

in packages that don't need complex recipes to compute it. Let me know what is the general feeling about it.

adamjstewart · 2017-01-18T13:46:18Z

I like that idea. Things that rarely need access to the spec, like build_directory, should be attributes.

davydden · 2017-01-18T15:38:23Z

looks good, but i must admit i did not went through it thoroughly.

adamjstewart · 2017-01-18T15:46:02Z

Out of curiosity, did you run into the problem I described here? I'm assuming that if you can compile m4, then you're good to go. I am curious how you got past this though.

alalazo · 2017-01-18T15:49:53Z

Out of curiosity, did you run into the problem I described here? I'm assuming that if you can compile m4, then you're good to go. I am curious how you got past this though.

@adamjstewart Yes, and I compiled out of source. All the autotools packages have been modified this way and compile on my ubuntu 14.04.

adamjstewart · 2017-01-18T15:52:20Z

Should we consider always building Autotools packages out of source by default like we do for CMakePackage? I'm trying to remember whether or not I've run into an Autotools package that could not be built out of source, but I guess I've never tried.

alalazo · 2017-01-18T15:58:53Z

@adamjstewart I would like to say yes, but as far as I can tell most of the projects written for autotools won't compile if configured out of source. Not a fault of the tool I guess, rather bad habits taken from the community using it. Bottomline: I wouldn't change the default...

tgamblin · 2017-01-18T17:58:11Z

@alalazo: @krafczyk has been thinking about building more packages out of source, in the context of Spack as a development environment. I think he might like the stuff you're doing here.

This is mostly OT for this PR, but I'm curious about whether we could provide a more general way to expose out of source builds to Spack. The idea would be to give the user or site control over where the source gets staged, and take that out of the hands of the packager. e.g., we could provide a variable called source_dir, something like this:

with working_directory(source_dir):
    # do build

or a special context handler:

with out_of_source():
    # do build

The idea would be that the framework passes in the source directory. Packages could say whether they support out of source builds with a top-level package attribute:

class Foo(Package):
    builds_out_of_source = True

Why do this? This would allow you to stage the source in the install prefix, so that it can stay after the build for use by debuggers and tools. @mplegendre and I had talked about this a while ago, but we never got around to implementing a mechanism for it.

Potential pitfalls: this wouldn't work for all packages, but it would be a start. It doesn't handle things like keeping generated code around for debuggers -- those would still go in the build directory and get removed afterwards. So maybe something more general is needed. @mplegendre and I talked about building in place, copying source files to the prefix, and rewriting file paths in DWARF, but that is more complicated than what is possible with a little build system support.

Anyway, not trying to divert the thread, just throwing this out there.

tgamblin · 2017-01-18T18:08:27Z

@alalazo: back on topic, I like the idea of making the simple stuff into properties. build_directory is unlikely to be a statically known path, though -- it likely depends on the spec... in one way or another.

alalazo · 2017-01-18T18:18:31Z

@tgamblin But if we build in the stage, as we do, we can pass in relative paths that are just strings and make them absolute in the base class. This is what I do here. Don't know if there are cases where that would create problems...

tgamblin · 2017-01-18T18:20:15Z

@alalazo: true, that could work. worst case we could provide some known format strings that culd be used for things that need to be subbed in later, e.g. '{stage_root}/directory_alongside_tarball'

alalazo · 2017-01-18T18:21:52Z

@tgamblin Good idea. Maybe we can reuse what @scheibelp has done already for spec substitution if need be.

adamjstewart · 2017-01-19T20:20:41Z

var/spack/repos/builtin/packages/tk/package.py

@@ -52,7 +52,7 @@ def setup_environment(self, spack_env, run_env):
        run_env.set('TK_LIBRARY', join_path(self.prefix.lib, 'tk{0}'.format(
            self.spec.version.up_to(2))))

-    def build_directory(self):
+    def configure_directory(self):
        return 'unix'


This and tcl need to be converted from functions to attributes.

Thanks, missed those two.

adamjstewart · 2017-01-19T20:24:01Z

lib/spack/spack/build_systems/python.py

@@ -97,10 +97,11 @@ def configure(self, spec, prefix):

    extends('python')

-    def setup_file(self, spec, prefix):
+    def setup_file(self):


Why this particular change? This function is only needed for a single file, and that package needs to access the spec.

We might want to save things that change between self and self/spec/prefix for another PR. You, @tgamblin, and I all disagree on the best way to handle this 😆

Because it's not a phase... but I would have pointed out the change before tagging this ready (if you didn't precede me to it) 😊

Ah, I see. Since it's only a single file, I don't mind as much. But it is annoying to try to convert things to use configure_args and look out for whether or not they use spec. I think I'm more interested in convenience and you're more interested in maintaining a well-designed system. There are merits to both, obviously.

Leave the change in for now. You've convinced me, at least for this case.

I think I'm more interested in convenience and you're more interested in maintaining a well-designed system. There are merits to both, obviously.

dependency injection = convenience + modular design

adamjstewart · 2017-01-19T20:29:06Z

lib/spack/spack/build_systems/autotools.py

+            m.aclocal()
+            m.autoconf()
+            autogen = Executable('./autogen.sh')
+            autogen()


I know this is how swiftsim does things, but is this generic enough for other packages? Most packages that need it seem to run something like:

autoreconf('--install', '--verbose', '--force')

Is your method a more robust version of the same thing?

We may want to comment out the autoreconf method I added to AutoreconfPackageTemplate in spack create, just to let users know that there is now a default and it should suffice most of the time. I would be interested in seeing how many of the other packages in Spack that need to generate a configure script could be converted to use the default.

The last time I tried to generate a configure script, a lot of m4 modules were missing. Autoreconf generated some warnings, but I think it went ok. If possible, we should figure out what is actually necessary to get rid of those warnings. Packages like libquo, for example, explicitly define where automake and libtool can be found.

I'll dig deeper into this but I think autogen.sh does an:

autoreconf('--install', '--verbose', '--force')

plus other things. The first two calls should update files in the configure directory if need be. If you have other cases than swiftsim at hand I can try them in this PR to see if I can come up with a recipe that works for the most part of them.

According to docs:

aclocal adds m4 macros that may be needed by configure.ac in the folder aclocal.m4

libtoolize provides a way to add support for libtool

autoconf creates a new configure from aclocal.m4 and configure.ac

autoreconf does a few of the above things repeatedly to generate a configure (but I remember I saw cases were the order was not right and plain autoreconf was failing)

to my surprise autogen.sh seems not to be a generated file, but an hand maintained one, even though it is mentioned in the FAQ.

Given all of the above I would remove:

autogen()

and substitute it with:

autoreconf('--install', '--verbose', '--force')

and hope that @mathstuf and colleagues will take over the market with CMake. Does it sound good @adamjstewart ?

Thanks for doing some research @alalazo. I would say, take the packages currently in Spack that call autoreconf and see how many of them will work with your new default method. Go by whatever works for most packages.

alalazo · 2017-01-25T21:37:58Z

@adamjstewart @davydden @tgamblin and others. Seeing the pattern here while working on this (in particular 634805c) I am tempted to push:

depends_on('autoconf', type='build')
depends_on('automake', type='build')
depends_on('libtool', type='build')
depends_on('m4', type='build')

into the base AutotoolsPackage class. What is your preference on this? On the one hand it may add unnecessary dependencies on the package, but on the other these dependencies won't change the hash and are very likely to be there anyhow.

adamjstewart · 2017-01-25T21:40:31Z

@alalazo I would rather add an AutoreconfPackage class that subclasses AutotoolsPackage and adds an autoreconf phase as well as these dependencies.

alalazo · 2017-01-25T21:42:17Z

@adamjstewart This doesn't solve the issue. There are packages that need the phase at some version and doesn't need them at some other version.

davydden · 2017-01-25T21:50:37Z

push.. into the base AutotoolsPackage class. What is your preference on this?

I don't have anything against it as long as those dependencies build robustly enough not to break every AutotoolsPackage which may not even need them. From that perspective, it is probably a safer bet to add another class AutoreconfPackage while keeping AutotoolsPackage only for those packages where we know that re-configure is not required for any version.

adamjstewart · 2017-01-25T21:54:05Z

... as long as those dependencies build robustly enough not to break every AutotoolsPackage ...

As far as I know, GCC, Intel, PGI, and NAG can build all of these packages and their dependencies, so I think they are fairly robust, for what it's worth.

davydden · 2017-01-25T21:58:01Z

they are fairly robust, for what it's worth.

that's generally my experience as well.

alalazo · 2017-01-25T21:58:31Z

@davydden See the argument above for AutoreconfPackage. There's also another: I think complexity should be hidden in the framework. Asking a user to distinguish between AutoreconfPackage and AutotoolsPackage requires more knowledge that the actual state of affairs, and doesn't go in the direction of making things simpler for a packager.

I don't have anything against it as long as those dependencies build robustly enough not to break every AutotoolsPackage which may not even need them.

That's a very good point (even though if those won't build I think you won't be able to build a lot of stuff). Another option is to rediscuss when we will have CDash + package testing in place

davydden · 2017-01-25T22:02:35Z

Another option is to rediscuss when we will have CDash + package testing in place

as long as we do tests before 1.0, the extra dependencies can be added straight away.
In the worst case scenario one would remove them again if things are fragile.

adamjstewart · 2017-01-25T22:05:57Z

Asking a user to distinguish between AutoreconfPackage and AutotoolsPackage requires more knowledge that the actual state of affairs, and doesn't go in the direction of making things simpler for a packager.

I agree that simplicity is important, although spack create now distinguishes between AutotoolsPackage and AutoreconfPackage automatically, so it wouldn't be such a big impact on simplicity.

alalazo · 2017-01-25T22:11:21Z

I agree that simplicity is important, although spack create now distinguishes between AutotoolsPackage and AutoreconfPackage automatically, so it wouldn't be such a big impact on simplicity.

@adamjstewart I think I miss that change! Well, after this PR I hope it will be rarely necessary to override autoreconf as most of the logic that recurred in packages has been pushed to the base class. There are exceptions of course, but I guess that otherwise it won't be hpc 😭

…urce. The configure script executable is now invoked with an absolute path. Modified a few packages accordingly.

alalazo · 2017-01-26T09:01:08Z

I just understood I overlooked a thing in the last issue I raised: circular dependencies on the tools themselves (autoconf depends on autoconf and so on...). I'll leave this PR as it is and consider it ready from my side.

spack create now distinguishes between AutotoolsPackage and AutoreconfPackage automatically, so it wouldn't be such a big impact on simplicity.

@adamjstewart I took a brief look at spack create, and I don't agree with the statement above. The fact that you have an AutoreconfPackageTemplate it's just an implementation detail that helps you (as a core developer) make a better guess to help packagers. As a packager you are not exposed to it. If we ever create an AutoreconfPackage class that would be exposed to a packager.

This argument anyhow comes in order of importance after the other one: some packages needs both behaviors.

JavierCVilla · 2017-01-30T13:24:51Z

@alalazo I got this error while installing libmng:

...
==> Already patched libmng
==> Building libmng [AutotoolsPackage]
==> Executing phase : 'autoreconf'
==> Executing phase : 'configure'
==> Error: ProcessError: Command exited with status 1:
    '/my/path/spack/var/spack/stage/libmng-2.0.2-2x5fkukzf3sf4uexegr3n35jwmy5pclu/libmng-2.0.2/configure' '--prefix=/my/path/spack/opt/spack/linux-scientificcernslc6-x86_64/gcc-6.2.0/libmng-2.0.2-2x5fkukzf3sf4uexegr3n35jwmy5pclu'
/my/path/spack/lib/spack/spack/build_systems/autotools.py:265, in configure:
     258      def configure(self, spec, prefix):
     259          """Runs configure with the arguments specified in :py:meth:`.configure_args`
     260          and an appropriately set prefix.
     261          """
     262          options = ['--prefix={0}'.format(prefix)] + self.configure_args()
     263  
     264          with working_dir(self.build_directory, create=True)

And this is the spack-build.out:

...
checking for a BSD-compatible install... /usr/bin/install -c
checking whether build environment is sane... yes
checking for a thread-safe mkdir -p... /bin/mkdir -p
checking for gawk... gawk
checking whether make sets $(MAKE)... yes
checking whether make supports nested variables... yes
configure: error: source directory already configured; run "make distclean" there first

Before this merge I could install it correctly but now I get this, do you think that could be related with the commit or maybe I have to install it in a different way now?

davydden · 2017-01-30T13:55:34Z

@JavierCVilla i think it's better to open a new issue for that.

* AutotoolsPackage: added configure_directory to permit build out of source. The configure script executable is now invoked with an absolute path. Modified a few packages accordingly. * build_systems: functions returning directories are now properties * build_systems: fixed issues with tcl and tk * AutotoolsPackage: reworked recipe for autoreconf

alalazo added autotools feature labels Jan 18, 2017

alalazo requested review from davydden and adamjstewart January 18, 2017 13:39

adamjstewart reviewed Jan 19, 2017

View reviewed changes

alalazo added the WIP label Jan 19, 2017

alalazo force-pushed the features/autotools_package_minor_improvements branch 3 times, most recently from 6d2f7b3 to 634805c Compare January 25, 2017 21:32

alalazo added 4 commits January 26, 2017 09:48

AutotoolsPackage: added configure_directory to permit build out of so…

5d8399f

…urce. The configure script executable is now invoked with an absolute path. Modified a few packages accordingly.

build_systems: functions returning directories are now properties

996acad

build_systems: fixed issues with tcl and tk

349a31f

AutotoolsPackage: reworked recipe for autoreconf

8371ee7

alalazo force-pushed the features/autotools_package_minor_improvements branch from 634805c to 8371ee7 Compare January 26, 2017 08:52

alalazo added ready and removed WIP labels Jan 26, 2017

tgamblin merged commit 81a5146 into spack:develop Jan 26, 2017

alalazo deleted the features/autotools_package_minor_improvements branch January 26, 2017 11:30

JavierCVilla mentioned this pull request Jan 30, 2017

Fail to build libmng [AutotoolsPackage] #2959

Closed

alalazo mentioned this pull request Jan 30, 2017

libmng: fix trying to run configure on an already configured directory #2961

Merged

JavierCVilla mentioned this pull request Jan 31, 2017

Fixing problem calling a property in PythonPackage #2968

Closed

scheibelp mentioned this pull request Nov 3, 2017

AutotoolsPackage Openmpi with clang on os x #6139

Closed

AutotoolsPackage: minor improvements #2859

AutotoolsPackage: minor improvements #2859

Conversation

alalazo commented Jan 18, 2017

Modifications:

alalazo commented Jan 18, 2017

adamjstewart commented Jan 18, 2017

davydden commented Jan 18, 2017

adamjstewart commented Jan 18, 2017

alalazo commented Jan 18, 2017

adamjstewart commented Jan 18, 2017

alalazo commented Jan 18, 2017

tgamblin commented Jan 18, 2017

tgamblin commented Jan 18, 2017 • edited

alalazo commented Jan 18, 2017

tgamblin commented Jan 18, 2017 • edited

alalazo commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alalazo commented Jan 25, 2017

adamjstewart commented Jan 25, 2017

alalazo commented Jan 25, 2017

davydden commented Jan 25, 2017

adamjstewart commented Jan 25, 2017

davydden commented Jan 25, 2017

alalazo commented Jan 25, 2017 • edited

davydden commented Jan 25, 2017

adamjstewart commented Jan 25, 2017

alalazo commented Jan 25, 2017

alalazo commented Jan 26, 2017

JavierCVilla commented Jan 30, 2017

davydden commented Jan 30, 2017

tgamblin commented Jan 18, 2017 •

edited

tgamblin commented Jan 18, 2017 •

edited

alalazo commented Jan 25, 2017 •

edited