Added a sub-command to show if packages are relocatable by alalazo · Pull Request #9199 · spack/spack

alalazo · 2018-09-10T16:28:47Z

This implements the idea discussed in the ticket above. From user perspective a sample output is:

$ spack buildcache preview hdf5
Relocatable nodes
--------------------------------
[-]  [email protected]%[email protected]~cxx~debug~fortran~hl~mpi+pic+shared+szip~threadsafe arch=linux-ubuntu18.04-x86_64 
[+]      ^[email protected]%[email protected] arch=linux-ubuntu18.04-x86_64 
[+]      ^[email protected]%[email protected]+optimize+pic+shared arch=linux-ubuntu18.04-x86_64

meaning that in this DAG hdf5 might not be relocatable, while libszip and zlib surely are.

alalazo · 2018-09-10T16:33:29Z

This PR still needs work to make the sub-command portable. Anyhow I wanted to submit it early so that people could start to play with it, and check if their packages of interest are or are not 100% relocatable. For instance, a worrying thing is:

$ spack buildcache preview mpich
Relocatable nodes
--------------------------------
[-]  [email protected]%[email protected] device=ch3 +hydra netmod=tcp +pmi+romio~verbs arch=linux-ubuntu18.04-x86_64

I suspect most of the times Spack's root directory (or any subfolder) is present in the data segment just to be used in help strings, but I can't think of a reliable way to ensure that the package remains relocatable.

gartung

Looks OK to me.

gartung · 2018-09-10T16:39:59Z

@alalazo Tk is the only package that I know of the uses a compiled in path to locate its config file. Other strings can probably be safely ignored. This is why I added the option (-a) to create the buildcache anyway.

tgamblin

@alalazo: this looks great to me but coverage is only 35%.

Could we make a test that builds trivial relocatable and non-relocatable binaries? I think this would be the first spack test to actually compile something, but it seems worth it to me. We could skip the test if clang or gcc is unavailable via spack compilers. What do you think?

alalazo · 2018-09-16T06:31:35Z

Could we make a test that builds trivial relocatable and non-relocatable binaries? I think this would be the first spack test to actually compile something, but it seems worth it to me. We could skip the test if clang or gcc is unavailable via spack compilers. What do you think?

Definitely worth to come up with some tests. Also, pointing this out to be sure you saw it: this PR currently works only for linux, as I don't have a MacOS to try something similar:

spack/lib/spack/spack/relocate.py

Lines 497 to 499 in 5ac4665

    
           if platform.system().lower() != 'linux': 
        
               msg = 'function currently implemented only for linux' 
        
               raise NotImplementedError(msg)

alalazo · 2018-09-19T10:12:56Z

To proceed with this PR I had to write something that skips tests if I don't find certain executables:

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

Lines 38 to 50 in 45f2065

    
           @pytest.fixture(autouse=True) 
        
           def _skip_if_missing_executables(request): 
        
               """Permits to mark tests with 'require_executables' and skip the 
        
               tests if the executables passed as arguments are not found. 
        
               """ 
        
               if request.node.get_marker('requires_executables'): 
        
                   required_execs = request.node.get_marker('requires_executables').args 
        
                   missings_execs = [ 
        
                       x for x in required_execs if spack.util.executable.which(x) is None 
        
                   ] 
        
                   if missings_execs: 
        
                       msg = 'could not find executables: {0}' 
        
                       pytest.skip(msg.format(', '.join(missings_execs)))

It's used like this:

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

Lines 81 to 89 in 45f2065

    
           @pytest.mark.requires_executables( 
        
               '/usr/bin/gcc', 'patchelf', 'strings', 'file' 
        
           ) 
        
           def test_file_is_relocatable(source_file, is_relocatable): 
        
               compiler = spack.util.executable.which('/usr/bin/gcc') 
        
               executable = str(source_file).replace('.c', '.x') 
        
               compiler(str(source_file), '-o', executable) 
        
               assert spack.relocate.file_is_relocatable(executable) is is_relocatable

I was wondering if we should move this up the test folder, maybe in conftest.py. Could this be useful elsewhere?

alalazo · 2018-09-19T18:17:51Z

@tgamblin I think I addressed your comments here, let me know if other changes are necessary.

alalazo · 2018-09-30T20:35:29Z

ping

alalazo · 2018-10-21T19:52:52Z

@tgamblin Rebased and ready for a further review.

alalazo · 2018-11-14T15:18:02Z

@tgamblin

pramodk · 2018-12-08T16:11:14Z

I suspect most of the times Spack's root directory (or any subfolder) is present in the data segment just to be used in help strings, but I can't think of a reliable way to ensure that the package remains relocatable.

We recently started using buildcache. I see that common packages like gcc, python, sqlite are not relocatable if we just check with strings. I wonder if there should be a property in in package.py that we can start adding (as we test packages) which can override checks like strings.

alalazo · 2018-12-10T08:32:15Z

I see that common packages like gcc, python, sqlite are not relocatable if we just check with strings.

As far as I understand packages that contain strings referring to the installation path can't be insured to be relocatable. If you have <prefix> in the data segment I assume there's no way to check if you are using it "just" to display text to a user or if you are using the path as part of your application logic.

I wonder if there should be a property in in package.py that we can start adding (as we test packages) which can override checks like strings.

Do you mean override the check so that it passes the query on relocatability, while with strings it does not? As long as we don't have reliable ways to substitute strings in a binary I don't know if that's a good idea. We would just hide the issue while not solving it.

That said, my understanding of this subject is quite naive at the moment. There are a couple of projects, like exodus, that I want to have a look at to see how they approach the issue. Also: if anybody has good references on the subject of relocating binaries, feel free to post them here (I would be glad to read them).

gartung

This looks OK to me. You will have to rebase on develop once all of the other changes to buildcache.py and relocate.py go in.

alalazo · 2019-01-15T20:01:50Z

@gartung @tgamblin Rebased and ready for another review.

gartung · 2019-02-22T16:00:30Z

I got around to testing this as an alternative to --allow-root. It tries to run on all files under /usr/ for external packages. The mime type misidentifies files as ELF that are not.

==> 'file' '-b' '--mime-type' '/build/work/spack-data/install/linux-rhel7-x86_64/gcc-7.3.0/root-6.12.06-i7e2sbt2pstdfhfm7tzgwf4z6hnnguke/etc/allDict.cxx.pch'
==> [MIME_TYPE] /build/work/spack-data/install/linux-rhel7-x86_64/gcc-7.3.0/root-6.12.06-i7e2sbt2pstdfhfm7tzgwf4z6hnnguke/etc/allDict.cxx.pch -> application/octet-stream
==> [/build/work/spack-data/install/linux-rhel7-x86_64/gcc-7.3.0/root-6.12.06-i7e2sbt2pstdfhfm7tzgwf4z6hnnguke/etc/allDict.cxx.pch] -> BINARY FILE
==> 'strings' '/build/work/spack-data/install/linux-rhel7-x86_64/gcc-7.3.0/root-6.12.06-i7e2sbt2pstdfhfm7tzgwf4z6hnnguke/etc/allDict.cxx.pch'
==> 'patchelf' '--print-rpath' '/build/work/spack-data/install/linux-rhel7-x86_64/gcc-7.3.0/root-6.12.06-i7e2sbt2pstdfhfm7tzgwf4z6hnnguke/etc/allDict.cxx.pch'
not an ELF executable

lib/spack/spack/relocate.py

…l or external packages. (#229)

gartung · 2019-02-22T21:54:09Z

@tgamblin I think this is ready for merging.
@alalazo Binaries which are built with debug info will have the source file path from the build stage. This usually still contains spack.store.layout.root so those packages will come up as not relocatable.

Test coverage is now substantially improved as requested

alalazo · 2019-02-28T19:12:36Z

lib/spack/spack/test/relocate.py

+    assert spack.relocate.file_is_relocatable(executable) is is_relocatable
+
+
+@pytest.mark.skipif(


@gartung Can this skip_if be removed now that you added an implementation for darwin?

lib/spack/spack/relocate.py

…unctions introduced in spack#9199 Instead of replacing rpaths with placeholder and then checking strings, make use of the functions relocate.is_recocatable and relocate.is_file_relocatable to decide if a package needs the allow-root option. This fixes a problem where the placeholder path was not in the first rpath entry. This was seen in c++ libraries and binaries because the compiler was outside the spack install base path and always appears first in the rpath. Instead of checking the first rpath entry, all rpaths have the placeholder path and the old install path (if it exists) replaced with the new install path. The second commit changes the check for old install path strings by using the install prefix instead of the install root.

…unctions introduced in spack#9199 Instead of replacing rpaths with placeholder and then checking strings, make use of the functions relocate.is_recocatable and relocate.is_file_relocatable to decide if a package needs the allow-root option. This fixes a problem where the placeholder path was not in the first rpath entry. This was seen in c++ libraries and binaries because the compiler was outside the spack install base path and always appears first in the rpath. Instead of checking the first rpath entry, all rpaths have the placeholder path and the old install path (if it exists) replaced with the new install path.

…rpath entry. * Rework of buildcache creation and install prefix checking using the functions introduced in #9199 Instead of replacing rpaths with placeholder and then checking strings, make use of the functions relocate.is_recocatable and relocate.is_file_relocatable to decide if a package needs the allow-root option. This fixes a problem where the placeholder path was not in the first rpath entry. This was seen in c++ libraries and binaries because the compiler was outside the spack install base path and always appears first in the rpath. Instead of checking the first rpath entry, all rpaths have the placeholder path and the old install path (if it exists) replaced with the new install path. * flake8

alalazo added don't-merge-yet buildcache binary-packages labels Sep 10, 2018

alalazo requested review from gartung and tgamblin September 10, 2018 16:28

gartung reviewed Sep 10, 2018

View reviewed changes

gartung approved these changes Sep 10, 2018

View reviewed changes

tgamblin approved these changes Sep 15, 2018

View reviewed changes

tgamblin previously requested changes Sep 15, 2018

View reviewed changes

alalazo force-pushed the features/buildcache_preview branch 2 times, most recently from f726b45 to 45f2065 Compare September 19, 2018 10:06

alalazo force-pushed the features/buildcache_preview branch 5 times, most recently from d52c5c6 to af0380e Compare September 19, 2018 17:29

alalazo removed the don't-merge-yet label Sep 30, 2018

alalazo force-pushed the features/buildcache_preview branch from af0380e to c070293 Compare October 21, 2018 19:10

alalazo force-pushed the features/buildcache_preview branch from c070293 to 97cc493 Compare December 10, 2018 09:18

gartung approved these changes Jan 11, 2019

View reviewed changes

alalazo added 2 commits January 15, 2019 17:05

Added tests for the 'buildcache preview' command.

2644a44

Fixed codebase after rebase

91c3e1f

alalazo force-pushed the features/buildcache_preview branch from 97cc493 to 91c3e1f Compare January 15, 2019 16:33

Fixed the list of apt addons for Python 3.7 in travis.yaml

255deea

gartung mentioned this pull request Feb 15, 2019

Buildache install fix: replace placeholder and origin install paths in rpath when relocating binaries #10287

Closed

Merge branch 'develop' into features/buildcache_preview

ab89c04

gartung requested changes Feb 22, 2019

View reviewed changes

lib/spack/spack/relocate.py Show resolved Hide resolved

gartung reviewed Feb 22, 2019

View reviewed changes

lib/spack/spack/relocate.py Show resolved Hide resolved

gartung approved these changes Feb 22, 2019

View reviewed changes

gartung mentioned this pull request Feb 22, 2019

Updates to #9199: Adding a preview subcommand to buildcache to show if a package is relocatable. #10687

Closed

gartung and others added 3 commits February 22, 2019 18:50

Only check ELF executables and shared libraries. Skip checking virtua…

e68672d

…l or external packages. (#229)

Fixed flake8 issues

b196861

Add handling for macOS mach binaries (#231)

2c9865a

gartung requested a review from tgamblin February 22, 2019 22:36

This was referenced Feb 28, 2019

Feature: buildcache relocation with different layout and compiler location #10522

Closed

Buildcache updates: Resubmit of #10287 #10756

Merged

scheibelp self-assigned this Feb 28, 2019

alalazo commented Feb 28, 2019

View reviewed changes

scheibelp reviewed Feb 28, 2019

View reviewed changes

lib/spack/spack/relocate.py Show resolved Hide resolved

gartung merged commit e3af8ed into spack:develop Feb 28, 2019

gartung mentioned this pull request Feb 28, 2019

Mark a package as non-relocatable #7237

Closed

alalazo deleted the features/buildcache_preview branch March 1, 2019 06:09

alalazo mentioned this pull request Jul 17, 2023

buildcache push always --allow-root #38878

Merged

		assert spack.relocate.file_is_relocatable(executable) is is_relocatable


		@pytest.mark.skipif(

Conversation

alalazo commented Sep 10, 2018

Uh oh!

alalazo commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gartung left a comment

Choose a reason for hiding this comment

Uh oh!

gartung commented Sep 10, 2018

Uh oh!

tgamblin left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo commented Sep 16, 2018

Uh oh!

alalazo commented Sep 19, 2018

Uh oh!

alalazo commented Sep 19, 2018

Uh oh!

alalazo commented Sep 30, 2018

Uh oh!

alalazo commented Oct 21, 2018

Uh oh!

alalazo commented Nov 14, 2018

Uh oh!

pramodk commented Dec 8, 2018

Uh oh!

alalazo commented Dec 10, 2018

Uh oh!

gartung left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo commented Jan 15, 2019

Uh oh!

gartung commented Feb 22, 2019

Uh oh!

Uh oh!

Uh oh!

gartung commented Feb 22, 2019

Uh oh!

alalazo Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gartung Feb 28, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alalazo commented Sep 10, 2018 •

edited

Loading

alalazo Feb 28, 2019 •

edited

Loading