Packages can tune the list of files to be archived at the end of install #7760

alalazo · 2018-04-14T13:19:35Z

This PR introduces a new attribute for packages that contains a list of glob expressions. Any file that matches will be archived in the <prefix>/.spack/archived-files directory.

AutotoolsPackage and CMakePackage provide a sensible default override for this attribute.

healther

One variable name nitpick and failing to archive files should only happen in very rare cases... I'm not sure if we should suppress the exception.
But don't consider these remarks blocking

healther · 2018-04-14T14:05:19Z

lib/spack/spack/package.py

+        with working_dir(self.stage.source_path):
+            for glob_expr in self.archive_files:
+                files = glob.glob(glob_expr)
+                for file in files:


typically we try to avoid overriding builtin functions. Not sure how critical this is though...

healther · 2018-04-14T14:06:52Z

lib/spack/spack/package.py

+                        # Here try to be conservative, and avoid discarding
+                        # the whole install procedure because of copying a
+                        # single file failed
+                        tty.warn("FAILED ARCHIVE: {0}".format(file))


failing to copy a file to a place we control means that something bad happened... maybe this should be a configurable option?

I think this should get recorded in the .spack directory rather than spit out as a warning. Maybe keep track of what was missing and spit out one warning and write .spack/missing.txt at the end?

I am detecting two kinds of errors:

Glob expressions that are relative to folders outside the source path

Errors when copying a matching file

They'll be recorded in .spack/archived-files/errors.txt. In case there's any error, we print a warning and point the user to that file.

adamjstewart

Oh wait, shouldn't these prepend self.build_directory? There are quite a few packages that override the defaultbuild_directory:

$ grep -l build_directory */package.py | xargs grep -l AutotoolsPackage
adios/package.py
autoconf/package.py
automake/package.py
berkeley-db/package.py
bison/package.py
coreutils/package.py
datamash/package.py
elpa/package.py
gawk/package.py
gcc/package.py
gmake/package.py
gnutls/package.py
guile/package.py
hpx5/package.py
ipopt/package.py
libtool/package.py
log4cxx/package.py
m4/package.py
mosh/package.py
mozjs/package.py
patch/package.py
python/package.py
swig/package.py
tcl/package.py
wget/package.py
yambo/package.py
$ grep -l build_directory */package.py | xargs grep -l CMakePackage
catalyst/package.py
cbtf-argonavis/package.py
cbtf-krell/package.py
cbtf-lanl/package.py
cbtf/package.py
clamr/package.py
funhpc/package.py
googletest/package.py
llvm/package.py
metis/package.py
openspeedshop/package.py
pfunit/package.py
phist/package.py
py-espressopp/package.py
qmcpack/package.py
rr/package.py
simulationio/package.py

tgamblin

Some nitpicks.

tgamblin · 2018-04-14T14:41:42Z

lib/spack/spack/package.py

+                        # Here try to be conservative, and avoid discarding
+                        # the whole install procedure because of copying a
+                        # single file failed
+                        tty.warn("FAILED ARCHIVE: {0}".format(file))


I think this should get recorded in the .spack directory rather than spit out as a warning. Maybe keep track of what was missing and spit out one warning and write .spack/missing.txt at the end?

tgamblin · 2018-04-14T14:44:19Z

lib/spack/spack/build_systems/autotools.py

@@ -94,6 +94,9 @@ class AutotoolsPackage(PackageBase):
    #: Options to be passed to autoreconf when using the default implementation
    autoreconf_extra_args = []

+    #: Files to archive for packages based on autotools
+    archive_files = ['config.log']


This will be wrong for some packages because config.log is typically relative to where configure runs. But we know that -AutotoolsPackage has vars for it. Should we allow substitutions?

@adamjstewart @tgamblin Absolutely right. I have extended this behavior, and now defaults should be treated correctly even for packages that override build_directory

alalazo · 2018-04-15T11:42:17Z

@adamjstewart @tgamblin The non-exceptional paths should be hit during the build tests on Travis. Was wondering if:

We should add a check there to ensure that config.log and CMakeLists.txt are in the correct place
We should add tests (how?) also for the exceptional paths

Let me know what do you think.

alalazo · 2018-04-20T09:22:41Z

@tgamblin ping

scheibelp · 2018-05-03T02:08:12Z

was wondering if...
We should add a check there to ensure that config.log and CMakeLists.txt are in the correct place

To be clear, are you saying that it could print an error message if config.log isn't found where it is expected? That would be useful but not required.

We should add tests (how?) also for the exceptional paths

IMO this would be worthwhile as long as it is simple: you could add a mock package definition to the test package repo (and the install method could create the test files which need to be moved). Beyond line coverage I think that would be useful to add a test which confirms that the archive_files specified by a package are saved when they are available. Let me know if that seems like more effort than it's worth or unclear.

fixes spack#2781 This PR introduces a new attribute for packages that contains a list of glob expressions. Any file that matches will be archived in the `<prefix>/.spack/archived-files` directory. `AutotoolsPackage` and `CMakePackage` provide a sensible default override for this attribute.

The glob expressions that can be used in the `archive_files` list now are also allowed to be absolute. The errors that may occur when archiving custom files are collected and reported in a file named `errors.txt`.

This ensures that errors when archiving files are recorded into .spack/archived-files/errors.txt

alalazo · 2018-05-05T18:24:39Z

@tgamblin @scheibelp I added unit tests, let me know if you think other cases should be tested or if there are other changes to be done in this PR.

scheibelp · 2018-05-08T01:42:50Z

var/spack/repos/builtin.mock/packages/a/package.py

@@ -54,6 +54,10 @@ class A(AutotoolsPackage):

    depends_on('b', when='foobar=bar')

+    @property


@alalazo I really appreciate the addition of the unit test. In my opinion though it would be worthwhile to create a new mock package for testing. Although this creates some redundancy, I think it avoids potential issues with confusing the purpose of unit tests when multiple tests use the same mock package. If that seems sensible to you then I'll wait for this to be separated out; otherwise let me know and I'll check with other folks internally to see if there's a general consensus (if most folks disagree with me I'll accept this as is).

@scheibelp I'll create a new mock package, no worries. Usually I reuse what's there because adding new mock packages requires more lines than I would like.

If that seems sensible to you then I'll wait for this to be separated out;

👍

@scheibelp done

Requests are now addressed

scheibelp · 2018-05-09T00:40:59Z

Thanks!

alalazo added feature autotools cmake labels Apr 14, 2018

alalazo requested review from tgamblin, scheibelp, healther, davydden and adamjstewart April 14, 2018 13:19

davydden approved these changes Apr 14, 2018

View reviewed changes

adamjstewart approved these changes Apr 14, 2018

View reviewed changes

healther approved these changes Apr 14, 2018

View reviewed changes

adamjstewart requested changes Apr 14, 2018

View reviewed changes

tgamblin previously requested changes Apr 14, 2018

View reviewed changes

adamjstewart approved these changes Apr 15, 2018

View reviewed changes

alalazo mentioned this pull request Apr 24, 2018

Tracking changes of our perpetual release candidate epfl-scitas/spack#128

Closed

9 tasks

alalazo added 3 commits May 5, 2018 14:29

Archive files default take into account the build directory

07c8350

The glob expressions that can be used in the `archive_files` list now are also allowed to be absolute. The errors that may occur when archiving custom files are collected and reported in a file named `errors.txt`.

Added a unit test to ensure that extra files are archived properly

d084326

alalazo force-pushed the features/copy_config_log_in_prefix branch from ce8580f to d084326 Compare May 5, 2018 13:22

Modified unit test to hit exceptional paths

f6bebeb

This ensures that errors when archiving files are recorded into .spack/archived-files/errors.txt

scheibelp reviewed May 8, 2018

View reviewed changes

Moved logic to test archiving files into its own mock package

d02f9fb

scheibelp merged commit b4859e1 into spack:develop May 9, 2018

alalazo deleted the features/copy_config_log_in_prefix branch May 9, 2018 06:10

scheibelp mentioned this pull request May 18, 2018

Querying a package's license (and from its dependencies) ? #8168

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packages can tune the list of files to be archived at the end of install #7760

Packages can tune the list of files to be archived at the end of install #7760

alalazo commented Apr 14, 2018

healther left a comment

healther Apr 14, 2018

healther Apr 14, 2018

tgamblin Apr 14, 2018

alalazo Apr 15, 2018

adamjstewart left a comment

tgamblin left a comment

tgamblin Apr 14, 2018

tgamblin Apr 14, 2018

alalazo Apr 15, 2018

alalazo commented Apr 15, 2018

alalazo commented Apr 20, 2018

scheibelp commented May 3, 2018

alalazo commented May 5, 2018

scheibelp May 8, 2018

alalazo May 8, 2018

alalazo May 8, 2018

scheibelp commented May 9, 2018

		@@ -54,6 +54,10 @@ class A(AutotoolsPackage):

		depends_on('b', when='foobar=bar')

		@property

Packages can tune the list of files to be archived at the end of install #7760

Packages can tune the list of files to be archived at the end of install #7760

Conversation

alalazo commented Apr 14, 2018

healther left a 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

adamjstewart left a comment

Choose a reason for hiding this comment

tgamblin left a 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 Apr 15, 2018

alalazo commented Apr 20, 2018

scheibelp commented May 3, 2018

alalazo commented May 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheibelp commented May 9, 2018