stage: make source_path available before stage is built
#11528
Conversation
tgamblin
changed the title
|
@chissg: this is 2.7 from the HEP requests... note still WIP so you may want to wait. |
tldahlgren
force-pushed
the
features/usable-source-path
branch
2 times, most recently
from
2f147cf
to
3ef3b53
Compare
tgamblin
changed the title
source_path property available before stage is built
tgamblin
changed the title
source_path property available before stage is builtsource_path available before stage is built
There was a problem hiding this comment.
@tldahlgren: looks mostly good to me! And the tests pass.
I put in some refactor requests.
Also, the patch coverage is 62% -- I think we should get this part of the code to 100% as it's core to Spack. Can you add tests for the parts that aren't covered? The codecov pages load slowly but you should be able to look at the diff coverage through the codecov links at the bottom of the PR.
lib/spack/spack/test/conftest.py
Outdated
| @@ -515,7 +515,7 @@ def mock_archive(tmpdir_factory): | |||
| tar = spack.util.executable.which('tar', required=True) | |||
|
|
|||
| tmpdir = tmpdir_factory.mktemp('mock-archive-dir') | |||
| expanded_archive_basedir = 'mock-archive-repo' | |||
| expanded_archive_basedir = spack.stage._source_path_subdir | |||
There was a problem hiding this comment.
this probably doesn't need its own variable anymore (it's used for the last time two lines down)
lib/spack/spack/test/stage.py
Outdated
| # TMPDIR/ temp stage dir | ||
| # src/ well-known stage source directory | ||
| # README.txt test_readme (contains "hello world!\n") | ||
| # test-files.tar.gz archive_url = file:///path/to/this |
There was a problem hiding this comment.
nitpick: make it say archive_url = file:///path/to/test-files.tar.gz to avoid confusion about this
lib/spack/spack/test/stage.py
Outdated
| # Expect the fetch area to look like: | ||
| # | ||
| # TMPDIR/ | ||
| # test-files.tar.gz archive_url = file:///path/to/this |
lib/spack/spack/stage.py
Outdated
| @@ -410,7 +403,7 @@ def generate_fetchers(): | |||
| continue | |||
| except spack.error.SpackError as e: | |||
| tty.msg("Fetching from %s failed." % fetcher) | |||
| tty.debug(e) | |||
| tty.debug("%s: %s" % (e.__class__.__name__, str(e))) | |||
There was a problem hiding this comment.
we could (should?) make tty.debug do this for all exceptions...
There was a problem hiding this comment.
It can be useful to see exactly which exception is being raised.
There was a problem hiding this comment.
I added a number of these in one of the commits I pushed on May 30, 2019.
lib/spack/spack/fetch_strategy.py
Outdated
| @@ -474,7 +500,14 @@ def check(self): | |||
| @_needs_stage | |||
| def expand(self): | |||
| tty.debug( | |||
| "Source fetched with %s is already expanded." % self.url_attr) | |||
| "Moving source fetched with %s to %s." % (self.url_attr, | |||
| self.stage.source_path)) | |||
There was a problem hiding this comment.
This is probably too detailed for a user message -- they shouldn't have to care that the source was moved.
There was a problem hiding this comment.
Do the users typically enable debugging messages?
There was a problem hiding this comment.
If they have issues, we ask the to run spack —debug to enable these. But generally they just want to know that things are installed (and maybe why — i.e. what dependent caused the install, which we currently don’t show).
There was a problem hiding this comment.
They don't - it has to be enabled with spack -d <command>. We typically ask for the debug output of this command when they report an issue. I think it can relate to internals; if it shouldn't, then I think we should create a logging level where we can report internal state, for our own sanity when debugging an issue.
There was a problem hiding this comment.
Let's make it debug for now and revisit. We had a conversation with @adamjstewart on another issue about simplifying tty to have fewer logging levels, but I actually think what we may need is to actually use a level between debug and msg more frequently. I think a lot of the things we currently print out should be saved in a log somewhere that can be automatically attached to issues.
See here: #11516 (comment)
lib/spack/spack/fetch_strategy.py
Outdated
| # Move the directory to the well-known stage source path | ||
| stage_entries = os.listdir(self.stage.path) | ||
| assert len(stage_entries) == 1 | ||
| repo_root = os.path.join(self.stage.path, stage_entries[0]) |
There was a problem hiding this comment.
These three lines should be in some function -- they're used a lot. Maybe repo _root = self._ensure_one_stage_entry(). We have things like one_spec_or_raise elsewhere.
lib/spack/spack/fetch_strategy.py
Outdated
| stage_entries = os.listdir(self.stage.path) | ||
| assert len(stage_entries) == 1 | ||
| repo_root = os.path.join(self.stage.path, stage_entries[0]) | ||
| shutil.move(repo_root, self.stage.source_path) |
There was a problem hiding this comment.
This is good but I think we can do better. git, svn, and hg can all checkout directly to a DEST path (the last args in each command below). I think those fetch strategies can now just be tweaked to clone into source_path, and we can eliminate the move logic from VCSFetchStrategy.
$ git clone -h
usage: git clone [<options>] [--] <repo> [<dir>]
$ hg clone -h
hg clone [OPTION]... SOURCE [DEST]
$ svn checkout -h
checkout (co): Check out a working copy from a repository.
usage: checkout URL[@REV]... [PATH]
There was a problem hiding this comment.
Great catch! Thanks.
lib/spack/spack/fetch_strategy.py
Outdated
| shutil.move(src, self.stage.source_path) | ||
| if len(files) > 1: | ||
| files.remove(non_hidden[0]) | ||
| source_root = os.path.dirname(self.stage.source_path) |
There was a problem hiding this comment.
Isn't this just self.stage.path?
lib/spack/spack/fetch_strategy.py
Outdated
| shutil.move(os.path.join(tarball_container, f), | ||
| os.path.join(self.stage.path, f)) | ||
| # TODO: Is it correct to assume the hidden files should be moved | ||
| # TODO: to the root of the stage source directory? |
There was a problem hiding this comment.
yep they should stay alongside the expanded directory. Note that this is probably something you need to account for in these recurring lines:
stage_entries = os.listdir(self.stage.path)
assert len(stage_entries) == 1
repo_root = os.path.join(self.stage.path, stage_entries[0])
So, in other words, shouldn't he logic here, to check for a single non-hidden directory, be used at the root of the stage to account for the case where you've already moved these files up?
I would make this into a function and use the same logic in the tarball_container and in self.stage.path. I think it'll reduce the complexity for this special case too.
There was a problem hiding this comment.
I'm not quite following the issue here.
The non-hidden file checks do not apply if the source was already staged. When they do apply, it is to the tarball_container, os.path.join(self.stage.path, "spack-expanded-archive"), after the archive file is decompressed into it.
Actually, why is the self.stage.expanded check deferred to after the tarball is decompressed? It seems more efficient to skip all the decompress-related code in this case.
There was a problem hiding this comment.
I refactored the above function per the initial comment; however, its use is no longer relevant/critical to ensure only one stage directory entry (for moving "pre-expanded" git, hg, or svn fetch strategy content) due the shift to use output directory options when cloning/checking out repositories.
|
FYI. I'm still working on code coverage associated with the principle files modified by this PR. |
tldahlgren
force-pushed
the
features/usable-source-path
branch
3 times, most recently
from
8639861
to
8835b6d
Compare
Codecov Report
@@ Coverage Diff @@
## develop #11528 +/- ##
===========================================
- Coverage 75.09% 75.01% -0.09%
===========================================
Files 214 214
Lines 23429 23394 -35
Branches 4589 4579 -10
===========================================
- Hits 17595 17549 -46
- Misses 4718 4731 +13
+ Partials 1116 1114 -2
Continue to review full report at Codecov.
|
tldahlgren
force-pushed
the
features/usable-source-path
branch
from
1ca3047
to
88805fa
Compare
lib/spack/spack/fetch_strategy.py
Outdated
| tty.msg("Cannot find version %s in url_list" % pkg.version) | ||
|
|
||
| except BaseException: | ||
| except BaseException as e: # pragma: no cover |
There was a problem hiding this comment.
why no cover? In general we wan to know when things are covered or not...
There was a problem hiding this comment.
Fair enough. Why? I likely had trouble building or coming up with a test case for it.
| @@ -328,14 +328,14 @@ def build_tarball(spec, outdir, force=False, rel=False, unsigned=False, | |||
| except Exception as e: | |||
| shutil.rmtree(workdir) | |||
| shutil.rmtree(tarfile_dir) | |||
| tty.die(str(e)) | |||
| tty.die("%s: %s" % (e.__class__.__name__, str(e))) | |||
There was a problem hiding this comment.
I think I suggested this, but what I meant was that we should change this in the tty module, so that if a single exception argument is passed in, we print the classname and the stringified exception, instead of just the stringified exception. Can you move this to tty? It'll reduce the clutter in this PR.
We generally try to keep ancillary changes out of the same PR (unless the commits are well factored), but I think I caused this so let's keep the tty change in.
There was a problem hiding this comment.
Okay. I misunderstood your suggestion. That sounds like a great idea.
lib/spack/spack/stage.py
Outdated
| @@ -41,13 +42,14 @@ def _first_accessible_path(paths): | |||
|
|
|||
| # ensure accessible | |||
| if not can_access(path): | |||
| continue | |||
| continue # pragma: no cover | |||
lib/spack/spack/stage.py
Outdated
| # fight over shared temporary space. | ||
| user = getpass.getuser() | ||
| if user not in path: | ||
| path = os.path.join(path, user, 'spack-stage') | ||
| path = os.path.join(path, user, 'spack-stage') # pragma: no cover |
lib/spack/spack/fetch_strategy.py
Outdated
| git(*(args + ['--depth', '1', self.url, | ||
| self.stage.source_path])) | ||
| cloned = True | ||
| except spack.error.SpackError as e: # pragma: no cover |
lib/spack/spack/test/stage.py
Outdated
| # <_readme_fn> test_readme (contains _readme_contents) | ||
| # <_extra_fn> test_extra file (contains _extra_contents) | ||
| # <_archive_fn> archive_url = file:///path/to/<_archive_fn> | ||
| # |
There was a problem hiding this comment.
Given that the new names now apply to more than one test, I'd move the descriptive comment up with the module-scope vars.
lib/spack/spack/test/stage.py
Outdated
| monkeypatch.setattr(spack.stage, '_use_tmp_stage', False) | ||
| yield | ||
| monkeypatch.setattr(spack.stage, '_tmp_root', current_root) | ||
| monkeypatch.setattr(spack.stage, '_use_tmp_stage', current_use_tmp_stage) |
There was a problem hiding this comment.
I believe that monkeypatch automatically reverts these (that's the point of monkeypatch), so you do not need to track the current values and restore them. Can you see if you can simplify this?
There was a problem hiding this comment.
I didn't want to assume but that's an excellent point. Will try.
| def tmpdir_for_stage(mock_archive): | ||
| """Uses a temporary directory for staging""" | ||
| current = spack.paths.stage_path | ||
| @pytest.fixture |
There was a problem hiding this comment.
Ok this is kind of cool. Didn't know pytest.fixture was written to work with or without parens. I checked the version of pytest that we vendor (it's old for 2.6 compatibility) and seems to work there too. Cool.
lib/spack/spack/test/url_fetch.py
Outdated
| assert fetcher is not None | ||
|
|
||
| with Stage(fetcher, path=testpath) as stage: | ||
| assert stage is not None |
There was a problem hiding this comment.
don't think this can happen ever.
There was a problem hiding this comment.
Ah. Great catch. Thanks!
lib/spack/spack/test/url_fetch.py
Outdated
| testpath = str(tmpdir) | ||
|
|
||
| fetcher = URLFetchStrategy(mock_archive.url) | ||
| assert fetcher is not None |
There was a problem hiding this comment.
remove these -- this can't happen.
|
Is this ready for another review? |
If you want. I removed |
|
@tldahlgren: I'm basically asking whether you're done. I think at this point we should try to wrap up the PR and move on to the next thing -- this fixes the bug and actually adds a lot of project coverage, so I just want to get the commits factored well and get this merged. The diff is only showing low coverage because of all the extra |
tgamblin
force-pushed
the
features/usable-source-path
branch
from
bbc71cf
to
a95b209
Compare
- make tty.msg, tty.info, etc. print the exception type and stringified message if the message argument is an exception. - simplify parts of the code that call tty.debug(str(e)) - add extra tty.debug statements in places where exceptions were previously ignored
- `stage.source_path` was previously overloaded; it returned `None` if it
didn't exist and this was used by client code
- we want to be able to know the `source_path` before it's created
- make stage.source_path available before it exists.
- use a well-known stage source path name, `$stage_path/src` that is
available when `Stage` is instantiated but does not exist until it's
"expanded"
- client code can now use the variable before the stage is created.
- client code can test whether the tarball is expanded by using the new
`stage.expanded` property instead of testing whether `source_path` is
`None`
- add tests for the new source_path semantics
tgamblin
force-pushed
the
features/usable-source-path
branch
from
a95b209
to
84ed733
Compare
|
@tldahlgren and I went through this and refactored commits so I think we're good now -- we can rebase this when it passes the tests. |
- keep this disabled until the `compiler find` fork bomb is fixed.
|
Merged! Thanks @tldahlgren |
| for f in files: | ||
| src = os.path.join(tarball_container, f) | ||
| dest = os.path.join(self.stage.path, f) | ||
| shutil.move(src, dest) |
There was a problem hiding this comment.
I suspect my issue #11645 is coming from these lines. Like the tarball is not being unpacked where it is expected to be? Just a guess.
This is also affecting other packages, example:
[osolberg@bifx1n03 spack]$ ./bin/spack install lua-luafilesystem
==> Installing libsigsegv
==> Searching for binary cache of libsigsegv
==> Warning: No Spack mirrors are currently configured
==> No binary for libsigsegv found: installing from source
==> Fetching https://ftpmirror.gnu.org/libsigsegv/libsigsegv-2.11.tar.gz
######################################################################## 100.0%
==> Staging archive: /home/osolberg/temp/spack/var/spack/stage/libsigsegv-2.11-qpmaxx6z62df4s4hwyehdwptv6kmrfhf/libsigsegv-2.11.tar.gz
==> Created stage in /home/osolberg/temp/spack/var/spack/stage/libsigsegv-2.11-qpmaxx6z62df4s4hwyehdwptv6kmrfhf
==> No patches needed for libsigsegv
==> Building libsigsegv [AutotoolsPackage]
==> Error: OSError: [Errno 2] No such file or directory: '/home/osolberg/temp/spack/var/spack/stage/libsigsegv-2.11-qpmaxx6z62df4s4hwyehdwptv6kmrfhf/src'
/home/osolberg/temp/spack/lib/spack/spack/package.py:1577, in build_process:
1574 echo = logger.echo
1575 self.log()
1576
>> 1577 # Run post install hooks before build stage is removed.
1578 spack.hooks.post_install(self.spec)
1579
1580 # Stop timer.
But src isn't at var/spack/stage/libsigsegv-2.11-qpmaxx6z62df4s4hwyehdwptv6kmrfhf, it's in var/spack/stage/libsigsegv-2.11-qpmaxx6z62df4s4hwyehdwptv6kmrfhf/libsigsegv-2.11:
[spack]$ ls -l var/spack/stage/libsigsegv-2.11-qpmaxx6z62df4s4hwyehdwptv6kmrfhf/
total 444
drwxr-xr-x 6 osolberg domain users 4096 Feb 18 2017 libsigsegv-2.11
-rw-r--r-- 1 osolberg domain users 448744 Jun 6 09:54 libsigsegv-2.11.tar.gz
[spack]$ ls -l var/spack/stage/libsigsegv-2.11-qpmaxx6z62df4s4hwyehdwptv6kmrfhf/libsigsegv-2.11/
total 828
-rw-r--r-- 1 osolberg domain users 43625 Feb 18 2017 aclocal.m4
-rw-r--r-- 1 osolberg domain users 242 Jan 2 2014 AUTHORS
drwxr-xr-x 2 osolberg domain users 128 Feb 18 2017 build-aux
-rw-r--r-- 1 osolberg domain users 49040 Feb 18 2017 ChangeLog
-rw-r--r-- 1 osolberg domain users 12345 Jan 2 2014 ChangeLog.1
-rw-r--r-- 1 osolberg domain users 4564 Feb 18 2017 config.h.in
-rw-r--r-- 1 osolberg domain users 4583 Feb 18 2017 config.h.msvc
-rwxr-xr-x 1 osolberg domain users 552750 Feb 18 2017 configure
-rw-r--r-- 1 osolberg domain users 31050 Feb 18 2017 configure.ac
-rw-r--r-- 1 osolberg domain users 18154 Jan 2 2014 COPYING
-rw-r--r-- 1 osolberg domain users 9678 Jan 2 2014 INSTALL
drwxr-xr-x 2 osolberg domain users 331 Feb 18 2017 m4
-rw-r--r-- 1 osolberg domain users 2830 Nov 23 2016 Makefile.am
-rw-r--r-- 1 osolberg domain users 28269 Feb 18 2017 Makefile.in
-rw-r--r-- 1 osolberg domain users 3385 Jan 2 2014 Makefile.msvc
-rw-r--r-- 1 osolberg domain users 4489 Feb 18 2017 NEWS
-rw-r--r-- 1 osolberg domain users 23457 Feb 18 2017 PORTING
-rw-r--r-- 1 osolberg domain users 6229 Feb 18 2017 README
-rw-r--r-- 1 osolberg domain users 10367 Nov 23 2016 README.windows
drwxr-xr-x 2 osolberg domain users 4096 Feb 18 2017 src
drwxr-xr-x 2 osolberg domain users 250 Feb 18 2017 tests
…#11648). Note that installing `catalyst` fails in the same place it does for the commit prior to merging changes for spack#11528.
Fixes spack#11528 See also spack#11648 Note that installing `catalyst` fails in the same place it does for the commit prior to merging changes for spack#11528.
See spack#11647 See spack#11528 Also added (missing) DIYStage unit tests to improve overall code coverage.
See spack#11647 See spack#11528 Also added (missing) DIYStage unit tests to improve overall code coverage.
#11528 updated Stage to always store a Package's source in a fixed directory accessible via `Stage.source_path` This left behind a number of packages which were expecting to access the source code via `Stage.path`. This Updates those packages to use `Stage.source_path` instead. This also updates the name of the fixed directory: The original name of the fixed directory was "src", so if an expanded archive created a "src" directory, then users inspecting the directory structure could see paths like "src/src" (which wasn't wrong but could be confusing). Therefore this also updates the name of the fixed directory to "spack-src".
…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'
Some tests introduced in spack#11528 temporarily set the user's `config:build_stage`, which affected (or created) a config.yaml file in the user's `$HOME/.spack` directory that could leave entries behind if the tests fail. This change ensures only temporary configuration files are used/affected by these tests.
Some tests introduced in #11528 temporarily set the user's `config:build_stage`, which affected (or created) a config.yaml file in the user's `$HOME/.spack` directory that could leave entries behind if the tests fail. This change ensures only temporary configuration files are used/affected by these tests.
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 ('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
spack#11528 updated Stage to always store a Package's source in a fixed directory accessible via `Stage.source_path` This left behind a number of packages which were expecting to access the source code via `Stage.path`. This Updates those packages to use `Stage.source_path` instead. This also updates the name of the fixed directory: The original name of the fixed directory was "src", so if an expanded archive created a "src" directory, then users inspecting the directory structure could see paths like "src/src" (which wasn't wrong but could be confusing). Therefore this also updates the name of the fixed directory to "spack-src".
Some tests introduced in spack#11528 temporarily set the user's `config:build_stage`, which affected (or created) a config.yaml file in the user's `$HOME/.spack` directory that could leave entries behind if the tests fail. This change ensures only temporary configuration files are used/affected by these tests.
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
spack#11528 updated Stage to always store a Package's source in a fixed directory accessible via `Stage.source_path` This left behind a number of packages which were expecting to access the source code via `Stage.path`. This Updates those packages to use `Stage.source_path` instead. This also updates the name of the fixed directory: The original name of the fixed directory was "src", so if an expanded archive created a "src" directory, then users inspecting the directory structure could see paths like "src/src" (which wasn't wrong but could be confusing). Therefore this also updates the name of the fixed directory to "spack-src".
Some tests introduced in spack#11528 temporarily set the user's `config:build_stage`, which affected (or created) a config.yaml file in the user's `$HOME/.spack` directory that could leave entries behind if the tests fail. This change ensures only temporary configuration files are used/affected by these tests.
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
The goal of this work is to make stage.source_path available before it exists. The approach (per @tgamblin) is to use a well-known stage source path name that is available when Stage is instantiated but does not exist until "expanded".
Well-behaved tarballs will be expanded such that the stage.source_path is the root directory (so hidden Mac OS X files are in the parent of stage.source_path). Exploding tarballs will be expanded in their respective subdirectories of stage.source_path. Un-expanded archives and other files (e.g., patches) will be moved to within stage.source_path.
This PR includes assorted documentation updates.