Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions lib/spack/spack/build_systems/autotools.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,11 @@ class AutotoolsPackage(PackageBase):
#: Options to be passed to autoreconf when using the default implementation
autoreconf_extra_args = []

@property
def archive_files(self):
"""Files to archive for packages based on autotools"""
return [os.path.join(self.build_directory, 'config.log')]

@run_after('autoreconf')
def _do_patch_config_guess(self):
"""Some packages ship with an older config.guess and need to have
Expand Down
5 changes: 5 additions & 0 deletions lib/spack/spack/build_systems/cmake.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ class CMakePackage(PackageBase):

depends_on('cmake', type='build')

@property
def archive_files(self):
"""Files to archive for packages based on CMake"""
return [os.path.join(self.build_directory, 'CMakeCache.txt')]

@property
def root_cmakelists_dir(self):
"""The relative path to the directory containing CMakeLists.txt
Expand Down
52 changes: 52 additions & 0 deletions lib/spack/spack/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import contextlib
import copy
import functools
import glob
import hashlib
import inspect
import itertools
Expand Down Expand Up @@ -529,6 +530,12 @@ class SomePackage(Package):
#: directories, sanity checks will fail.
sanity_check_is_dir = []

#: List of glob expressions. Each expression must either be
#: absolute or relative to the package source path.
#: Matching artifacts found at the end of the build process will
#: be copied in the same directory tree as build.env and build.out.
archive_files = []

#
# Set default licensing information
#
Expand Down Expand Up @@ -1647,8 +1654,53 @@ def log(self):
# FIXME : this potentially catches too many things...
pass

# Archive the whole stdout + stderr for the package
install(self.log_path, log_install_path)
# Archive the environment used for the build
install(self.env_path, env_install_path)
# Finally, archive files that are specific to each package
with working_dir(self.stage.source_path):
errors = StringIO()
target_dir = os.path.join(
spack.store.layout.metadata_path(self.spec), 'archived-files'
)
for glob_expr in self.archive_files:
# Check that we are trying to copy things that are
# in the source_path tree (not arbitrary files)
abs_expr = os.path.realpath(glob_expr)
if os.path.realpath(self.stage.source_path) not in abs_expr:
errors.write(
'[OUTSIDE SOURCE PATH]: {0}\n'.format(glob_expr)
)
continue
# Now that we are sure that the path is within the correct
# folder, make it relative and check for matches
if os.path.isabs(glob_expr):
glob_expr = os.path.relpath(
glob_expr, self.stage.source_path
)
files = glob.glob(glob_expr)
for f in files:
try:
target = os.path.join(target_dir, f)
# We must ensure that the directory exists before
# copying a file in
mkdirp(os.path.dirname(target))
install(f, target)
except Exception:
# Here try to be conservative, and avoid discarding
# the whole install procedure because of copying a
# single file failed
errors.write('[FAILED TO ARCHIVE]: {0}'.format(f))

if errors.getvalue():
error_file = os.path.join(target_dir, 'errors.txt')
mkdirp(target_dir)
with open(error_file, 'w') as err:
err.write(errors.getvalue())
tty.warn('Errors occurred when archiving files.\n\t'
'See: {0}'.format(error_file))

dump_packages(self.spec, packages_dir)

def sanity_check_prefix(self):
Expand Down
19 changes: 19 additions & 0 deletions lib/spack/spack/test/cmd/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -376,3 +376,22 @@ def test_install_mix_cli_and_files(clispecs, filespecs, tmpdir):

install(*args, fail_on_error=False)
assert install.returncode == 0


@pytest.mark.usefixtures(
'builtin_mock', 'mock_archive', 'mock_fetch', 'config', 'install_mockery'
)
def test_extra_files_are_archived():
s = Spec('a foobar=baz')
s.concretize()

install('a foobar=baz')

archive_dir = os.path.join(
spack.store.layout.metadata_path(s), 'archived-files'
)
config_log = os.path.join(archive_dir, 'config.log')
assert os.path.exists(config_log)

errors_txt = os.path.join(archive_dir, 'errors.txt')
assert os.path.exists(errors_txt)
10 changes: 8 additions & 2 deletions var/spack/repos/builtin.mock/packages/a/package.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ class A(AutotoolsPackage):

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

@property
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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;

👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scheibelp done

def archive_files(self):
return super(A, self).archive_files + ['../../outside.log']

def with_or_without_fee(self, activated):
if not activated:
return '--no-fee'
Expand All @@ -66,7 +70,9 @@ def configure(self, spec, prefix):
pass

def build(self, spec, prefix):
pass
mkdirp(self.build_directory)
config_log = join_path(self.build_directory, 'config.log')
touch(config_log)

def install(self, spec, prefix):
pass
touch(join_path(prefix, 'deleteme'))