Skip to content

Buildcache: Fix bug in binary string replacement#17075

Merged
gartung merged 3 commits intospack:developfrom
gartung:gartung-buildcache-text-bin-replacement-bug
Jun 12, 2020
Merged

Buildcache: Fix bug in binary string replacement#17075
gartung merged 3 commits intospack:developfrom
gartung:gartung-buildcache-text-bin-replacement-bug

Conversation

@gartung
Copy link
Copy Markdown
Member

@gartung gartung commented Jun 12, 2020 •
edited
Loading

Only the prefixes of the package dependencies and the package itself should be replaced. Replacing the spack directory results in an incorrect directory when the install root is a subdirectory of the spack directory,

@gartung gartung linked an issue Jun 12, 2020 that may be closed by this pull request
buildcache: relocation of binaries is broken when $padding is used in the install_tree config variable. #17041
Closed
@gartung gartung requested a review from alalazo June 12, 2020 17:41
@gartung gartung requested a review from eugeneswalker June 12, 2020 17:47
@eugeneswalker
Copy link
Copy Markdown
Contributor

eugeneswalker commented Jun 12, 2020

This resolves the issue! Thank you!

alalazo
alalazo reviewed Jun 12, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@alalazo alalazo left a comment

Choose a reason for hiding this comment

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

Is it doable to have a unit test for this bugfix?

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Jun 12, 2020 •
edited
Loading

If it helps you can have a look at:

spack/lib/spack/spack/test/relocate.py

Lines 351 to 373 in 4e39dc7

@pytest.mark.requires_executables('patchelf', 'strings', 'file', 'gcc')
def test_relocate_text_bin(hello_world, copy_binary, tmpdir):
orig_binary = hello_world(rpaths=[
str(tmpdir.mkdir('lib')), str(tmpdir.mkdir('lib64')), '/opt/local/lib'
], message=str(tmpdir))
new_binary = copy_binary(orig_binary)
# Check original directory is in the executabel and the new one is not
assert text_in_bin(str(tmpdir), new_binary)
assert not text_in_bin(str(new_binary.dirpath()), new_binary)
# Check this call succeed
spack.relocate.relocate_text_bin(
[str(new_binary)],
str(orig_binary.dirpath()), str(new_binary.dirpath()),
spack.paths.spack_root, spack.paths.spack_root,
{str(orig_binary.dirpath()): str(new_binary.dirpath())}
)
# Check original directory is not there anymore and it was
# substituted with the new one
assert not text_in_bin(str(tmpdir), new_binary)
assert text_in_bin(str(new_binary.dirpath()), new_binary)

in particular at the hello_world fixture, which will generate a binary with RPATHs or text of choice.

@gartung
Copy link
Copy Markdown
Member Author

gartung commented Jun 12, 2020

Can the test fixture generate a binary with
install_root: $spack/$padding:640

@gartung gartung merged commit 08c21e4 into spack:develop Jun 12, 2020
@eugeneswalker
Copy link
Copy Markdown
Contributor

eugeneswalker commented Jun 12, 2020

I agree we should do a test case. I can look into setting this up.

likask pushed a commit to likask/spack that referenced this pull request Jun 15, 2020
Merge commit '1501de59ed74802f48e32b0657fc6c95997b264a' into mofem
eda0329 
* commit '1501de59ed74802f48e32b0657fc6c95997b264a': (3648 commits)
  Package/py-lmfit: add new version (spack#16975)
  hpctoolkit: add version 2020.06.12 (spack#17081)
  add dependency for icd variant, or else build fails (spack#17079)
  clang: add 'version_argument', remove redundant method (spack#17071)
  New package: ocl-icd (spack#17078)
  Reframe 3.0 (spack#17005)
  py-healpy: a new package. (spack#17001)
  New recipe for building the Log4C package (spack#17038)
  fix depends issue and support for aarch64 (spack#17045)
  replace 'no' with 'none' as possible value of 'threads' variant (spack#17063)
  xrootd: new versions (spack#17076)
  add compilers to mpi setup_run_environment methods forall mpi implementations (spack#17015)
  bazel: patch to allow py-tensorflow (and likely other bazel packages) to build. (spack#17013)
  New package: FrontFlow Blue (spack#16901)
  cscope: Link tinfow instead of tinfo
  New package: alps (spack#17023)
  pygpu: fix linking with gpuarray (spack#17033)
  libtree package: add version 1.2.0, 1.1.4, and 1.1.3 (spack#17035)
  Buildcache: Fix bug in binary string replacement (spack#17075)
  New package: clinfo (spack#17042)
  ...

# Conflicts:
#	.gitignore
#	lib/spack/spack/binary_distribution.py
#	lib/spack/spack/modules/common.py
#	var/spack/repos/builtin/packages/med/package.py
#	var/spack/repos/builtin/packages/mofem-cephas/package.py
#	var/spack/repos/builtin/packages/mofem-fracture-module/package.py
#	var/spack/repos/builtin/packages/mofem-users-modules/package.py
#	var/spack/repos/builtin/packages/petsc/package.py
#	var/spack/repos/builtin/packages/python/package.py
manifestoso pushed a commit to DeepThoughtHPC/spack that referenced this pull request Jun 19, 2020
@gartung @manifestoso
Buildcache: Fix bug in binary string replacement (spack#17075)
b11a81f 
* Fix bug in binary string replacement that results in padding being added multiple times

* Update comment

* Update comment again
@alalazo alalazo mentioned this pull request Jul 7, 2020
Closed
@gartung
Copy link
Copy Markdown
Member Author

gartung commented Jul 27, 2020

@tgamblin @becker33 This is the other one that might not be in 0.15.

@gartung gartung deleted the gartung-buildcache-text-bin-replacement-bug branch July 27, 2020 18:34
@tgamblin
Copy link
Copy Markdown
Member

tgamblin commented Jul 27, 2020

@gartung: This one's already in. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alalazo alalazo alalazo left review comments

@eugeneswalker eugeneswalker eugeneswalker approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

buildcache: relocation of binaries is broken when $padding is used in the install_tree config variable.

4 participants

@gartung @eugeneswalker @alalazo @tgamblin