ResourceStage: use expanded archive name by default #11688

scheibelp · 2019-06-13T02:01:32Z

For resources, it is desirable to use the expanded archive name of the resource as the name of the directory when adding it to the root staging area.

#11528 established 'spack-src' as the universal directory where source files are placed, which also affected the behavior of resources managed with Stages.

This adds a new property to Stage to remember the name of the expanded source directory, and uses this as the default name when placing a resource directory in the root staging area.

This also:

Ensures that downloaded sources are archived using the expanded archive name
Updates working_dir context manager to guarantee restoration of original working directory when an exception occurs
Adds a "temp_cwd" context manager which creates a temporary directory and sets it as the working directory
~~Tests handling of exploding tarballs~~ (Edit 6/19: this was already handled)

TODOS:

Tests should be added to avoid regressions.
(New: 6/14) Fix python 2.6 errors

Note: this does not support resources using the go fetch strategy, but go fetching logic is currently in flux anyhow and no packages currently use go-based resources.

Note: this undoes some changes from #11676 and #11667 that accommodated the behavior changes from #11528. Those changes are compatible with this PR but I removed them because they are not strictly required (and in general the purpose of this PR was to reduce the amount of work required to use resources). In terms of staging, this PR would work with the logic for hpcviewer before or after #11676, but the hpcviewer changes are not undone because that PR updated the directory structure for that package build.

…source files are placed. For resources, it is desirable to use the expanded archive name as the default; therefore this stores the expanded archive name and uses it as the default for 'placement'

…der to allow resources to choose a sensible default name

lib/spack/spack/fetch_strategy.py

tldahlgren · 2019-06-18T16:42:31Z

lib/spack/spack/fetch_strategy.py

@@ -520,7 +526,16 @@ def archive(self, destination, **kwargs):
                tar.add_default_arg('--exclude=%s' % p)

        with working_dir(self.stage.path):
-            tar('-czf', destination, os.path.basename(self.stage.source_path))
+            if self.stage.srcdir:


Is this change addressing potential naming conflicts? Or ensuring any surviving archive has a meaningful name?

The latter: srcdir stores whatever meaningful name can be extracted (e.g. the basename of the URL). If one cannot be extracted, then we resort to creating a tarball that expands to spack-src/.

tldahlgren · 2019-06-18T16:44:26Z

lib/spack/spack/fetch_strategy.py

-            if not spack.config.get('config:debug'):
-                args.insert(1, '--quiet')
-            git(*args)
+            debug = spack.config.get('config:debug')


Why not use the git destination option here (as Todd wanted)?

Sorry I'm not familiar with that (the option or the context) - is this from a discussion in #11528?

Yes, see #11528 (comment).

I think the question is "why not check out the repository out directly to the spack-src directory?". The issue with that is that when we do that, we lose the repository name: for example if we do git clone https://github.com/spack then we get a directory called spack; this is useful when choosing a directory name for the archive in two cases:

This avoids putting resources named spack-src into a root stage

This avoids caching an SCM package into a directory called spack-src (i.e. when placed in var/spack/cache/)

Does that seem reasonable?

Ah. Thanks for the clarification.

tldahlgren · 2019-06-18T16:47:57Z

lib/spack/spack/fetch_strategy.py

+                # Yet more efficiency, only download a 1-commit deep tree
+                if self.git_version >= ver('1.7.1'):
+                    try:
+                        git(*(args + ['--depth', '1', self.url]))


Same question as above wrt using the git destination option.

tldahlgren · 2019-06-18T16:54:56Z

lib/spack/spack/fetch_strategy.py

@@ -828,8 +853,13 @@ def fetch(self):
        args = ['checkout', '--force', '--quiet']
        if self.revision:
            args += ['-r', self.revision]
-        args.extend([self.url, self.stage.source_path])
-        self.svn(*args)
+        args.extend([self.url])


Same question about using svn path (or destination) option as requested by Todd.

tldahlgren · 2019-06-18T16:56:16Z

lib/spack/spack/fetch_strategy.py

@@ -937,8 +967,13 @@ def fetch(self):
        if self.revision:
            args.extend(['-r', self.revision])

-        args.extend([self.url, self.stage.source_path])
-        self.hg(*args)
+        args.extend([self.url])


Same question wrt hg destination option as for git and svn.

lib/spack/spack/test/stage.py

Fixes #11816 Allow packages to refer to non-expanded downloads (e.g. a single script) using Stage.archive_file. This addresses a regression from #11688 and adds a unit test for it.

For resources, it is desirable to use the expanded archive name of the resource as the name of the directory when adding it to the root staging area. spack#11528 established 'spack-src' as the universal directory where source files are placed, which also affected the behavior of resources managed with Stages. This adds a new property ('srcdir') to Stage to remember the name of the expanded source directory, and uses this as the default name when placing a resource directory in the root staging area. This also: * Ensures that downloaded sources are archived using the expanded archive name (otherwise Spack will not be able to determine the original directory name when using a cached archive). * Updates working_dir context manager to guarantee restoration of original working directory when an exception occurs * Adds a "temp_cwd" context manager which creates a temporary directory and sets it as the working directory

Fixes spack#11816 Allow packages to refer to non-expanded downloads (e.g. a single script) using Stage.archive_file. This addresses a regression from spack#11688 and adds a unit test for it.

For resources, it is desirable to use the expanded archive name of the resource as the name of the directory when adding it to the root staging area. spack#11528 established 'spack-src' as the universal directory where source files are placed, which also affected the behavior of resources managed with Stages. This adds a new property ('srcdir') to Stage to remember the name of the expanded source directory, and uses this as the default name when placing a resource directory in the root staging area. This also: * Ensures that downloaded sources are archived using the expanded archive name (otherwise Spack will not be able to determine the original directory name when using a cached archive). * Updates working_dir context manager to guarantee restoration of original working directory when an exception occurs * Adds a "temp_cwd" context manager which creates a temporary directory and sets it as the working directory

Fixes spack#11816 Allow packages to refer to non-expanded downloads (e.g. a single script) using Stage.archive_file. This addresses a regression from spack#11688 and adds a unit test for it.

scheibelp added 13 commits June 12, 2019 18:41

spack#11528 established 'spack-src' as the universal directory where …

6922e40

…source files are placed. For resources, it is desirable to use the expanded archive name as the default; therefore this stores the expanded archive name and uses it as the default for 'placement'

Merge branch 'develop' into bugfix/default-resource-dest-dir

24b46ec

replace join_path with os.path.join for consistency

a4164e2

add test for default placement of expanding resources

3895905

add test for exploding tarball

9163d85

VCS fetch strategies need to record the default repository name in or…

784a4cc

…der to allow resources to choose a sensible default name

ensure restoration of state in context managers

401b45c

track repo name for other VCS fetch strategies

3e3fed8

eliminate unnecessary use of temp_cwd

bb55285

Don't condition save filename on whether the stage is expanded

a09f1fe

remove TODO now that test has been added for verification of behavior

947cffa

don't rename a directory if the new location matches the old

4d10e60

same-path check updated to account for symlinks

4f899ea

scheibelp changed the title ~~[WIP] ResourceStage: use expanded archive name by default~~ ResourceStage: use expanded archive name by default Jun 17, 2019

tldahlgren reviewed Jun 18, 2019

View reviewed changes

lib/spack/spack/fetch_strategy.py Outdated Show resolved Hide resolved

tldahlgren reviewed Jun 18, 2019

View reviewed changes

lib/spack/spack/test/stage.py Outdated Show resolved Hide resolved

undo changes for exploding tarball (it was already handled)

2dc2c73

scheibelp closed this Jun 19, 2019

scheibelp reopened this Jun 19, 2019

scheibelp merged commit 284ae9d into spack:develop Jun 20, 2019

scheibelp mentioned this pull request Jun 21, 2019

Reference non-expanding resources by "Stage.archive_file" #11817

Merged

scheibelp mentioned this pull request Jul 2, 2019

resource not in expected location #11666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ResourceStage: use expanded archive name by default #11688

ResourceStage: use expanded archive name by default #11688

scheibelp commented Jun 13, 2019 •

edited

Loading

tldahlgren Jun 18, 2019

scheibelp Jun 18, 2019

tldahlgren Jun 18, 2019

scheibelp Jun 18, 2019

tldahlgren Jun 18, 2019

scheibelp Jun 18, 2019

tldahlgren Jun 18, 2019

tldahlgren Jun 18, 2019

tldahlgren Jun 18, 2019

tldahlgren Jun 18, 2019

ResourceStage: use expanded archive name by default #11688

ResourceStage: use expanded archive name by default #11688

Conversation

scheibelp commented Jun 13, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheibelp commented Jun 13, 2019 •

edited

Loading