Conversation
lib/spack/spack/package.py
Outdated
| are no exceptions during build. Set to True to keep the stage | ||
| even with exceptions. | ||
| install_source (bool): By default, source is not installed, but | ||
| for debugging it might useful to keep it around. |
c59a5dd to
37ce47b
Compare
lib/spack/spack/package.py
Outdated
| if install_source: | ||
| src_target = join_path(self.spec.prefix, 'src') | ||
| tty.msg('Copying source to {0}'.format(src_target)) | ||
| install_tree(self.stage.source_path, src_target) |
There was a problem hiding this comment.
You'll probably want to check if the src directory actually exists first.
37ce47b to
d59139d
Compare
There was a problem hiding this comment.
Generally looks good. Some requests:
-
Can we call the option
--source?install --install-sourceseems redundant to me. -
Can you make the necessary modifications for this to be runnable after an existing install? e.g., say I install
fooand it gets mefoo /abc123. I should then be able to say:spack install --source /abc123and have it re-fetch, stage, patch, and add to the existing installation.
-
Can the source go in the
$prefix/.spack/srcdirectory so that it doesn't interfere with a package install? Putting it in$prefix/srcisn't really hier compliant. -
If you must have it in a non-dot location, it should probably go in
$prefix/share/<pkgname>/src, otherwise you're going to run into issues with symlinking inspack view, and once we add environments.
I'd really rather have the source go into .spack so that it's not included by default in the prefix --
I'd really rather have Spack prefixes be considered read-only and final once installed. The prefix is what's going to get turned into an installable binary, too -- so it makes sense to keep it as an immutable artifact. Basically if I give you a prefix hash, you should know exactly what to expect.
Does that seem reasonable?
How would I do that? |
citibeth
left a comment
There was a problem hiding this comment.
Can you please be specific about elucidating the original problem and use case driving this PR? I'm a little concerned that we've gotten to the stage of code review without knowing what the original problem is we're trying to solve.
|
@citibeth: see #4083 (comment) |
|
@tgamblin: I have done 1.) and 4.), I like having However for 2.) I will need some pointer in the code. ( @adamjstewart / @tgamblin ) |
citibeth
left a comment
There was a problem hiding this comment.
I'm "approving" because my request for a use case has been satisfied. I have unfortunately not reviewed the code.
lib/spack/spack/test/install.py
Outdated
| pkg.fetcher = fetcher | ||
|
|
||
|
|
||
| @pytest.mark.usefixtures('install_mockery') |
There was a problem hiding this comment.
It should run. All functions that start with test_ should be run. I'm not sure how often Codecov updates, if that's what you're asking.
There was a problem hiding this comment.
Hmm and the source_install=True below should trigger the new code path or does the mock_archive doesn't have source?
85ab25f to
930385e
Compare
|
Rebased: ping @tgamblin |
930385e to
a0bb530
Compare
|
Ping @tgamblin |
a0bb530 to
4250ee2
Compare
|
Ping @tgamblin |
4250ee2 to
18645b5
Compare
|
fixed flake8 errors. |
aa6ce30 to
b97d092
Compare
|
Added a test. @junghans: it's easier to do now than it was when you added this :) |
The tcl package references the original sources upon install, and the tk build requires the tcl sources. This updates the tcl package to install its sources following the example of #4102, and also updates the tclConfig.sh file to properly reference the installed sources (rather than the staging directory created by Spack).
Fix #4083.