spack setup: Fix Bugs + Multi-setup
#2664
Conversation
|
I'm looking for review on this PR... before Travis issues have been cleared. |
|
This will also address #2597 (where an error was popping up for some concretization after the first) |
|
Can you suggest what those tests should do? Maybe do a `spack setup` on
one package, and then go ahead and build it?
…On Thu, Dec 22, 2016 at 9:47 AM, Todd Gamblin ***@***.***> wrote:
***@***.**** requested changes on this pull request.
This PR should add regression tests for Spack setup's behavior.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2664 (review)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1cdygGnQ890wK7Y1FzhqmSK7cji3qvks5rKo2EgaJpZM4LTpXQ>
.
|
|
Does Spack currently have any regression tests that form a pattern I can follow? |
|
The whole |
lib/spack/spack/cmd/install.py
Outdated
| format = '$_' + '$@$%@+$+$=', | ||
| hashes = True, | ||
| hashlen = 7, | ||
| install_status = True) |
There was a problem hiding this comment.
Why has this been added to install?
|
|
||
| if len(specs) != 1: | ||
| tty.die('only one spec can be installed at a time.') | ||
| spec = specs.pop() |
There was a problem hiding this comment.
I don't get the change in arguments: why allow-multi? parse specs is supposed to be able to parse multiple specs. The fact that currently it is not is a bug (see #1248 and #1976).
Also, an error message that says that only one item can be installed doesn't seem appropriate in a function that parses.
There was a problem hiding this comment.
The docstring for parse_specs() indicates that it is a "convenience function." That means (to me) that it's not really necessary, but its entire purpose is to factor stuff out of places that use it, making other code neater and easier to deal with. Adding the allow_multi functionality to parse_specs() does exactly that, allowing the following lines to be factored out of at least 2 places:
if len(specs) != 1:
tty.die('only one spec can be installed at a time.')
spec = specs.pop()
return spec
Point taken on the error message.
The fact that currently it is not is a bug (see #1248 and #1976).
Bug or no bug, I took existing code and refactored it into a convenience function.
There was a problem hiding this comment.
allow_multi should be irrelevant now, the parser accepts multiple specs as of #2769
| def setup_parser(subparser): | ||
| subparser.add_argument( | ||
| '--only', | ||
| default='package,dependencies', | ||
| dest='things_to_install', | ||
| dest='only_str', |
There was a problem hiding this comment.
It might be me, but I don't get what the name only_str means in this context. Why did you change the name of this variable?
There was a problem hiding this comment.
only_str is consistent with the argument named only. Maybe this should be called only (except that only is really a set, but it's being encoded as a string)
lib/spack/spack/cmd/install.py
Outdated
| @@ -73,6 +77,11 @@ def setup_parser(subparser): | |||
| '--fake', action='store_true', dest='fake', | |||
| help="Fake install. Just remove prefix and create a fake file.") | |||
|
|
|||
| subparser.add_argument( | |||
| '--install-status', '-I', action='store_true', dest='install_status', | |||
| help="Show spec before installing.") | |||
There was a problem hiding this comment.
Why not:
spack spec -Il <spec>
spack install <spec>
?
There was a problem hiding this comment.
I agree, having two command which do the same thing is not good. I understand that spack spec may take some time for big packages, but IMO it's not a good enough reason to add this functionality to spack install.
There was a problem hiding this comment.
Because it takes 68 seconds instead of 34; and because I really want to use it routinely on every spack install or spack setup command. That way, I can see what Spack is doing while it's doing it, and I don't have to type twice as much and wait twice as long before I can check whether things are going OK. Why all the pushback on this one? It doesn't affect anybody who doesn't want to use it.
lib/spack/spack/cmd/install.py
Outdated
| package = spack.repo.get(s) | ||
| package.do_install(install_dependencies=True, explicit=False, **kwargs) | ||
|
|
||
| if install_package: |
There was a problem hiding this comment.
Not a big deal, but why we changed the semantics here? We are driving dependencies installation explicitly when it shouldn't be necessary: do_install knows how to recurse internally and here we are mixing two competing recursions on dependencies.
There was a problem hiding this comment.
This will get changed back.
| @@ -300,25 +310,25 @@ def install(parser, args, **kwargs): | |||
| if args.no_checksum: | |||
| spack.do_checksum = False # TODO: remove this global. | |||
|
|
|||
| only = set([x.strip() for x in args.only_str.split(',')]) | |||
There was a problem hiding this comment.
Minor thing: there's no need to turn the argument into a list. Invoking __contains__ will succeed also with the plain string.
Because for specs that take a long time to concretize, it is a real lifesaver.
I have no preferences. Sounds like (The |
|
@tgamblin I would like to wait for #2502 before writing the tests for this
PR. For the test itself, I would like two packages (B -> A) where B is
subclassed from `CMakePackage`. B should have a simple program that runs
and A should be a simple library, so we can go through a full (but short)
build process. I can then do `spack install`, `spack setup` and `spack
diy` on B, with different options for the `--only` flag.
Question: Is there a way I can include A.tar.gz and B.tar.gz (or
equivalent) so this test doesn't have to go to the Internet to run?
…On Thu, Dec 22, 2016 at 1:13 PM, Denis Davydov ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/spack/spack/cmd/install.py:
> @@ -73,6 +77,11 @@ def setup_parser(subparser):
'--fake', action='store_true', dest='fake',
help="Fake install. Just remove prefix and create a fake file.")
+ subparser.add_argument(
+ '--install-status', '-I', action='store_true', dest='install_status',
+ help="Show spec before installing.")
I agree, having two command which do the same thing is not good. I
understand that spack spec may take some time for big packages, but IMO
it's not a good enough reason to add this functionality to spack install.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#2664>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB1cd2wBO2OWuM09Mpugw9OPL1TzV5KMks5rKr3KgaJpZM4LTpXQ>
.
|
|
@citibeth: Sounds good to me. w.r.t. tests, look at |
citibeth
force-pushed
the
efischer/161221-FixSetup
branch
from
75d9d76
to
8d03f1b
Compare
|
I just pushed some significant changes that improve the The problem is... there is no guarantee that the version of BUT... there are also "legitimate" reasons for these hash problems. For example, if B Another similar setup is that (say) For a complex build with more than two packages you wish to At the end of the day, it's a way to waste hours digging through a mess. Which is what Spack is supposed to get us out of. This PR now offers a better way: generate all the Note that if you setup package The recent commits also make some other changes:
Below is an example of the "new" An |
|
Another change... CMake-specific stuff has now been factored into |
citibeth
changed the title
spack setup: Squash Bugs through Refactoringspack setup: Fix Bugs + Multi-setup
| @@ -49,6 +60,8 @@ class CMakePackage(PackageBase): | |||
| # build-system class we are using | |||
| build_system_class = 'CMakePackage' | |||
|
|
|||
| depends_on('cmake', type='build') | |||
There was a problem hiding this comment.
Can you rebase? This line is now present in develop.
|
@tgamblin It is my humble opinion that the structure set up for tests is needlessly complicated for this purpose, because the tests happen from WITHIN Spack. I would like a test like the following: I shouldn't have to get deeply involved with mock archives, mock packages, etc. just to do that. I do see the value in avoiding network access in tests. Can I write a test, external to Spack, that does the above? Do you have any suggestions? |
|
But not all commands can handle multiple specs. The convenience code here
is in place of command-level code that would previously throw an error if
more than one spec was discovered by the parser.
Unless/until all calls to this function have `allow_multi=True`, it should
remain, IMHO.
…On Wed, Jan 18, 2017 at 3:54 PM, becker33 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In lib/spack/spack/cmd/__init__.py
<#2664>:
>
except spack.spec.SpecError as e:
- tty.error(e.message)
- sys.exit(1)
+ tty.die(e.message)
+
+ if allow_multi:
+ return specs
+
+ if len(specs) != 1:
+ tty.die('only one spec can be installed at a time.')
+ spec = specs.pop()
allow_multi should be irrelevant now, the parser accepts multiple specs
as of #2769 <#2769>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2664>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AB1cdxz0I12yNG3ZF-liEmRlf3HbyHjmks5rTnwZgaJpZM4LTpXQ>
.
|
Seems reasonable to do in a separate PR. |
|
@tgamblin See latest for where I'm heading with the tests. I'm working on Suggestion no. 1 from above. Unfortunately, after merging with the latest develop I've broken the existing unit tests, and should probably get that fixed first. Do you know what is going wrong in the unit test log output below? 2 out of 516 tests are failing. |
|
It looks like these tests failed before my merge with |
|
OK I found the problem. |
Add everytrace to Mock repo; will be used for spack setup tests. Added unit test: setup
citibeth
force-pushed
the
efischer/161221-FixSetup
branch
from
84adc5c
to
5620838
Compare
|
@tgamblin OK unit tests pass. And I added a unit test that scrutinizes the generated If you want further tests, I'd be happy to build them on top of the example you suggest:
Please tell me what you think; this PR should be merged sooner rather than later. |
|
waiting for this PR, it would be great to have |
|
@tgamblin Does this PR need anything more before being merged? |
|
@tgamblin @alalazo @adamjstewart ping. Would be good to have |
|
Ping ping @tgamblin. This addresses multiple issues. |
There was a problem hiding this comment.
I'm taking over reviewing this from @tgamblin.
Some of these comments are actually just questions about how this works. This also needs to be brought up to date with the current develop branch, so I understand that some of my comments may be irrelevant when you do so.
Where I make a request, I'm curious whether you agree and - if you do - whether you have the time to address it. I imagine the most onerous request is for more tests.
| skip_patch=False, | ||
| verbose=False, | ||
| make_jobs=None, | ||
| run_tests=False, | ||
| fake=False, | ||
| explicit=False, | ||
| dirty=None, | ||
| setup=set(), |
There was a problem hiding this comment.
Elsewhere I state a preference that this be encoded as a member variable of package. But if you disagree with that:
Could you do one of the following:
- set the default to
Noneand replace with an empty set in the body? - set the default to
frozenset()
Otherwise IMO this is confusing since the defaults are evaluated once at definition time. To be clear I don't think this is an issue in your case since you don't modify the set, but making it a frozenset would make that explicit.
| # Abort install if install directory exists. | ||
| # But do NOT remove it (you'd be overwriting someone else's stuff) | ||
| tty.warn("Keeping existing install prefix in place.") | ||
| if self.name in setup: |
There was a problem hiding this comment.
I think this should just set keep_prefix=True no matter what. That seems to be what you want in your case and also what should happen anyway based on the comment.
| if self.name in setup: | ||
| this_fake = True | ||
| explicit = True | ||
| self.stage = DIYStage(os.getcwd()) # Force build in cwd |
There was a problem hiding this comment.
If setup was a property of Package, _make_stage could generate this stage instead of overwriting it which IMO would be clearer.
Also, a question: I'm not clear how we came to be in the build directory of the package. I presume that we aren't assuming that spack install is launched from the build directory of the package to be set up, since I'm not sure how that would work if two different packages were being set up.
| skip_patch=False, | ||
| verbose=False, | ||
| make_jobs=None, | ||
| run_tests=False, | ||
| fake=False, | ||
| explicit=False, | ||
| dirty=None, | ||
| setup=set(), | ||
| spconfig_fname_fn=None, # Must be set |
There was a problem hiding this comment.
I don't see spconfig_fname_fn being customized anywhere - from what I can tell this is always cmd.install.get_spconfig_fname. I'd prefer this was a property of Package, or possibly even a static function (since the method signature of do_install is getting a bit unwieldy).
| if fake: | ||
| if self.name in setup: | ||
| pass # Don't write any files in the install... | ||
| elif this_fake: |
There was a problem hiding this comment.
I can see how this works out but if if this_fake is used in this manner, I'd prefer it didn't also include the value of self.name in setup. i.e. above where you decide whether to skip patching/staging, check if fake or self.name in setup
|
|
||
| defs = dict() | ||
| defre = re.compile('-D(.*?)(:(.*?))?=(.*)') | ||
| print(spconfig.cmd) |
| os.environ = self.orig_environ | ||
| shutil.rmtree(self.dir) | ||
|
|
||
| def test_setup_cmake(self): |
There was a problem hiding this comment.
I think some additional tests would be worthwhile:
- Make sure the stage is not deleted if a
setuppackage fails to install - Make sure
Package.installis called for packages which should be set up
I could be wrong about those in particular but at the moment this only tests the integrity of the spconfig.py file, and it doesn't test any of the modifications to Package.do_install
| def spack_transitive_include_path(): | ||
| return ';'.join( | ||
| os.path.join(dep, 'include') | ||
| for dep in os.environ['SPACK_DEPENDENCIES'].split(os.pathsep) |
There was a problem hiding this comment.
Could you add a TODO comment to expose this in build_environment with a function so you don't have to grab it from the environment? I think that ought to be done but I don't think it needs to be done in this PR.
|
|
||
| def write_spconfig(self, spconfig_fname): | ||
| """Writes the spconfig.py (CMake setup file) to a file.""" | ||
| print('BEGIN write_spconfig') |
There was a problem hiding this comment.
Remove print statement (use tty.debug if you wan to see this somewhere)
|
@citibeth : would it be possible to rebase this on latest develop? I was trying to do it myself but don't have knowledge of all changes involved. |
|
@pramodskumbhar see #5043 for rebased version. I unfortunately do not currently have the time to push this PR over the finish line. I have put the new PR on the main LLNL Spak repo, in the hopes that someone else can test it, etc. and get it merged. I'm happy to answer questions, try it out on samples, etc. |
|
thank you very much @citibeth! No problem, I will test it and will let you know if I see any issues. |
|
Superseded by #5043 |
This PR addresses a serious case of "bitrot" in
spack setup: changes accumulated incmd/install.pyand ended up breakingcmd/setup.py.spack setupwas simply unusable. The main thrust of this PR is to makespack setupwork again.Benefits
spack setupis just plain broken #2662, and possibly other related issues of unstable hashes in the past.spack installshould print spec! #2661 (duplicate of Copy MacPorts UI: List dependencies to be installed before installing them! #2149).spack setup.Changes
Previously,
spack setupcalledspack installtwice, by passing it command line arguments.spack installin turn callsPackage.do_install(). Now,spack setupandspack installboth callPackage.do_install()directly.Previously, a call to
spack setupwould concretize three times. By not callingspack install,spack setupnow ony concretizes once. This is not just for efficiency, but also correctness. I suspect that concretizing is not idempotent; and that concretizing three times was a root cause of the bugs. This could have something to do with the way concretization results are memoized.Relevant parts of
spack installwere factored out into subroutines so thatspack setupcan make use of them.spack setupandspack installnow take the same command line arguments. All arguments were determined to make sense forspack setup.A new '-I' option was added that prints out the fully concretized spec before installing. If users want a more customized printout, they should use
spack spec.A context manager is now used to set up / tear down logging. This ensures that the logging tear-down will always be done, even in the face of a
sys.exit(1)ortty.die().To Do
spack setup,spack diyalso derives fromspack install. It should also be refactored to matchspack setupandspack install.spack install?