Buildcache fix text relocate #6512

gartung · 2017-11-29T04:45:40Z

Fixes for a few bugs identified recently.

*Text not relocating as described here
#6418

This was caused by passing a regular expression to filter_file. Passing a literal string for old and new install paths works. Added a test to make sure it works.

Binary files identified by file with "dynamically linked" were being skipped by a check for "link" in file type. Changed the check to "link to" to cover symbolic links.

files ending in ~ are removed by filter_file(backup=False) but the files might end up in the text to relocate list is filter_file() is called during install

Conflicts: lib/spack/spack/relocate.py

The previous test skipped dynamically linked in filetype and never tested for ELF. Add test for relocate_text to make sure problems are found in testing. Do not include the prefix directory name in tarball. This will aid later when installing to a different directory layout.

gartung · 2017-11-29T04:50:18Z

@sknigh @tgamblin @scheibelp

scheibelp · 2017-11-29T19:36:16Z

Closing and reopening to rerun tests now that #6513 is addressed

scheibelp

I had a question about the tar creation logic in build_tarball. Other than that this looks good.

scheibelp · 2017-11-30T03:43:18Z

lib/spack/spack/binary_distribution.py

@@ -256,7 +258,7 @@ def build_tarball(spec, outdir, force=False, rel=False, yes_to_all=False,
    # create compressed tarball of the install prefix
    with closing(tarfile.open(tarfile_path, 'w:gz')) as tar:
        tar.add(name='%s' % workdir,
-                arcname='%s' % os.path.basename(workdir))
+                arcname='.')


I think this was originally intended to use binary cached packages on Spack installations with different directory layouts. Is this useful for other purposes as well? I'm checking because IIRC you mentioned additional work needed to be done to support that.

No. To install in a different directory structure requires redoing the rpaths to the dependencies for the new directory structure. I don't see a way to do that the moment. I would rather focus on replacing paths compiled into object files first.

OK. If this change isn't being used for anything in this PR then I think it would be clearer to remove it (and reintroduce it in a PR which targets allowing relocation between different directory layouts).

Reverting this requires also backing out the way the tarball is unpacked and the way the workdir is named. I would rather leave it in.

Ah but I see that this breaks the unpacking of existing tarballs. I will revert and push the changes.

…tain compatability.

scheibelp

A couple extra questions

scheibelp · 2017-12-01T03:24:55Z

lib/spack/spack/binary_distribution.py

@@ -123,7 +125,7 @@ def write_buildinfo_file(prefix, rel=False):
    buildinfo['buildpath'] = spack.store.layout.root
    buildinfo['relocate_textfiles'] = text_to_relocate
    buildinfo['relocate_binaries'] = binary_to_relocate
-    filename = buildinfo_file_name(prefix)
+    filename = buildinfo_file_name(workdir)


What issue does this fix?

It clarifies things for me to distinguish the working copy from the original prefix. The buildinfo file is written to the working copy.

I added it to calculate the relative install prefix.

scheibelp · 2017-12-01T03:27:34Z

lib/spack/spack/binary_distribution.py

@@ -359,7 +361,8 @@ def relocate_package(prefix):
    path_names = set()
    for filename in buildinfo['relocate_textfiles']:
        path_name = os.path.join(prefix, filename)
-        path_names.add(path_name)
+        if '~' not in path_name:


Is this related to:

files ending in ~ are removed by filter_file(backup=False) but the files might end up in the text to relocate list is filter_file() is called during install

If so, IMO it would be clearer to use endswith because otherwise I was confused if you were looking for paths starting with ~ (i.e. referring to the home directory).

If you are wanting to remove paths which end with ~ (which I think is a backup file created by some editors) then IMO that isn't something you should have to deal with. If you think it ought to then IMO it is worth adding a comment about why this check is there.

I found one case where there was a file ending in ~ because filter_file was called during install. The file ended up on the list of text relocation files. It was overwritten when the parent file was changed and then removed.

Got it - filter_file itself generates these. Could you replace this with a call to endswith and add a comment about how this comes from filter_file?

…generated by filter_file()

…relocate

scheibelp · 2017-12-01T19:15:22Z

Thanks!

* Updated function which checks if a binary file needs relocation. Previously this was incorrectly identifying ELF binaries as symbolic links (so they were being excluded from relocation). Added test to check that ELF binaries are not considered symlinks. * relocate_text was not replacing paths in text files. Added test to check that text files are relocated properly (i.e. paths in the file are converted to the new prefix). * Exclude backup files created by filter_file when installing from binary cache. * Update write_buildinfo_file method signature to distinguish between the spec prefix and the working directory for the binary cache package.

* Revert "Quick fix for relocation issues." This reverts commit 57608a6. * Buildcache: relocate fixes (#6512) * Updated function which checks if a binary file needs relocation. Previously this was incorrectly identifying ELF binaries as symbolic links (so they were being excluded from relocation). Added test to check that ELF binaries are not considered symlinks. * relocate_text was not replacing paths in text files. Added test to check that text files are relocated properly (i.e. paths in the file are converted to the new prefix). * Exclude backup files created by filter_file when installing from binary cache. * Update write_buildinfo_file method signature to distinguish between the spec prefix and the working directory for the binary cache package.

* Revert "Quick fix for relocation issues." This reverts commit 57608a6. * Buildcache: relocate fixes (spack#6512) * Updated function which checks if a binary file needs relocation. Previously this was incorrectly identifying ELF binaries as symbolic links (so they were being excluded from relocation). Added test to check that ELF binaries are not considered symlinks. * relocate_text was not replacing paths in text files. Added test to check that text files are relocated properly (i.e. paths in the file are converted to the new prefix). * Exclude backup files created by filter_file when installing from binary cache. * Update write_buildinfo_file method signature to distinguish between the spec prefix and the working directory for the binary cache package.

@adamjstewart

* fixes #967 * Version bump to 0.9.1 - Bugfixes for spack find - 0.9.1 can read specs from current develop. * Don't assume spack is in the path when building docs. * Quick fix for relocation issues. * elf relocation fix: cherry-picked from develop branch (#6889) * Revert "Quick fix for relocation issues." This reverts commit 57608a6. * Buildcache: relocate fixes (#6512) * Updated function which checks if a binary file needs relocation. Previously this was incorrectly identifying ELF binaries as symbolic links (so they were being excluded from relocation). Added test to check that ELF binaries are not considered symlinks. * relocate_text was not replacing paths in text files. Added test to check that text files are relocated properly (i.e. paths in the file are converted to the new prefix). * Exclude backup files created by filter_file when installing from binary cache. * Update write_buildinfo_file method signature to distinguish between the spec prefix and the working directory for the binary cache package. * Final changes for v0.11.0 (#6318) * Fix logo link in README.md to point to the develop branch. (#6969) * Compiler flag handlers (#6415) This adds the ability for packages to apply compiler flags in one of three ways: by injecting them into the compiler wrapper calls (the default in this PR and previously the only automated choice); exporting environment variable definitions for variables with corresponding names (e.g. CPPFLAGS=...); providing them as arguments to the build system (e.g. configure). When applying compiler flags using build system arguments, a package must implement the 'flags_to_build_system_args" function. This is provided for CMake and autotools packages, so for packages which subclass those build systems, they need only update their flag handler method specify which compiler flags should be specified as arguments to the build system. Convenience methods are provided to specify that all flags be applied in one of the 3 available ways, so a custom implementation is only required if more than one method of applying compiler flags is needed. This also removes redundant build system definitions from tutorial examples * Fix type issues with setting flag handlers (#6960) The flag_handlers method was being set as a bound method, but when reset in the package.py file it was being set as an unbound method (all python2 issues). This gets the underlying function information, which is the same in either case. The bug was uncovered for parmetis in #6858. This is a partial fix. Included are changes to the parmetis package.py file to make use of flag_handlers. * Bump version to 0.11.1 * Added flags to unit tests + OSX build done once per day (#6988) * Adding flags to codecov reports * OSX builds are triggered once a day * Pull R list_urls from upstream. * travis: removed /usr/local/include/c++ before installing gcc on OSX (#6515) (#7027) "brew install gcc" fails for travis build because of an existing /usr/local/include/c++. This commit removes the offending file as suggested by brew. * Fix gfortran 7 detection (#7017) * Add NameError to exceptions caught from configure_args in module generation (#7173) * Revert "Binary caching: remove symlinks, copy files instead (#9747)" This reverts commit 058cf81. * Make Spack relocate text files in build caches with relative binaries * add the tfel package * fix the tfel package * fix the tfel package * fix the tfel package * Taking Adam J. Steward' remarks into account * fixes trailing white spaces * Update description * Update dependencies following @adamjstewart adices * Style fixes * Style fixes * Add java optional support * add the maintainers attribute (following @alalazo advice), disable interface not selected (following @adamjstewart advice) * flake8 fixes * Fix Cast3M and python-bindings support. Python detection is made compatible with cmake'FindPythonLibs module (at least how it is used in TFEL) * Style fixes * Style fixes * Fix test on python version * Follow @adamjstewart advices: code is much cleaner and readable * Small fix * Small fix * Add comment * Small fix in cmake option * try again (trying to overcome Travis CI unstable build process) * Add support for the MFrontGenericInterfaceSupport project (MGIS) * Style fixes * Package documentation update * Package documentation update * Fix a typo thanks to Andreas Baumbach review * Follow Adam J. Stewart advices * Fix type * bugfix: add back r's for invalid regexes * tutorial basics section: fix gcc install version * version bump: v0.12.1 * bugfix: bring in .travis.yml from develop * Add new TFEL' versions (3.0.4, 3.1.4 and 3.2.1). Add new MGIS version (1.0.1). Fix MGIS dependency * merge with spack:develop * add missing dependency * new versions of and * Fix MGIS url. Fix duplicate variant in TFEL * Fix tfel packaging according to Adam J. Stewart' advices * Fix flake8 warning Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com> Co-authored-by: Todd Gamblin <tgamblin@llnl.gov> Co-authored-by: Peter Scheibel <scheibel1@llnl.gov> Co-authored-by: Greg Becker <becker33@llnl.gov> Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>

* Revert "Quick fix for relocation issues." This reverts commit 57608a6. * Buildcache: relocate fixes (spack#6512) * Updated function which checks if a binary file needs relocation. Previously this was incorrectly identifying ELF binaries as symbolic links (so they were being excluded from relocation). Added test to check that ELF binaries are not considered symlinks. * relocate_text was not replacing paths in text files. Added test to check that text files are relocated properly (i.e. paths in the file are converted to the new prefix). * Exclude backup files created by filter_file when installing from binary cache. * Update write_buildinfo_file method signature to distinguish between the spec prefix and the working directory for the binary cache package.

gartung added 2 commits November 28, 2017 13:38

Use literal strings on regular expressions

fb9e8da

Conflicts: lib/spack/spack/relocate.py

gartung mentioned this pull request Nov 29, 2017

R installed from buildcache crashes #6418

Closed

scheibelp closed this Nov 29, 2017

scheibelp reopened this Nov 29, 2017

gartung mentioned this pull request Nov 29, 2017

Buildcache installed compilers cannot function correctly #5866

Closed

gartung added the buildcache label Nov 29, 2017

scheibelp reviewed Nov 30, 2017

View reviewed changes

Revert the change to how the tarball is created and extracted to main…

e2f4baf

…tain compatability.

gartung mentioned this pull request Dec 1, 2017

buildcache install should fail if the tarball was created with a different directory layout #6536

Closed

scheibelp reviewed Dec 1, 2017

View reviewed changes

Change test to endswith('~') to explicitly exclude backup text files …

1cc6141

…generated by filter_file()

gartung closed this Dec 1, 2017

gartung reopened this Dec 1, 2017

Merge remote-tracking branch 'llnl/develop' into buildcache-fix-text-…

cdf68c5

…relocate

scheibelp merged commit f6c99d7 into spack:develop Dec 1, 2017

scheibelp mentioned this pull request Mar 2, 2018

Attempt to deal with binary files which have strings compiled in other than rpaths. #7314

Merged

gartung deleted the buildcache-fix-text-relocate branch March 27, 2018 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Buildcache fix text relocate #6512

Buildcache fix text relocate #6512

gartung commented Nov 29, 2017 •

edited

gartung commented Nov 29, 2017

scheibelp commented Nov 29, 2017

scheibelp left a comment

scheibelp Nov 30, 2017

gartung Nov 30, 2017

scheibelp Nov 30, 2017

gartung Nov 30, 2017

gartung Nov 30, 2017

scheibelp left a comment

scheibelp Dec 1, 2017

gartung Dec 1, 2017

gartung Dec 1, 2017

scheibelp Dec 1, 2017

gartung Dec 1, 2017

scheibelp Dec 1, 2017

scheibelp commented Dec 1, 2017

Buildcache fix text relocate #6512

Buildcache fix text relocate #6512

Conversation

gartung commented Nov 29, 2017 • edited

gartung commented Nov 29, 2017

scheibelp commented Nov 29, 2017

scheibelp 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

Choose a reason for hiding this comment

scheibelp 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheibelp commented Dec 1, 2017

gartung commented Nov 29, 2017 •

edited