Explain and test two pkg dependency routines#11993
Conversation
Added comments to explain that `install_dependency_symlinks` and `flatten_dependencies` are actually used. Added a test that exercises the routines.
|
How can it claim the method is not covered when I added a test for it? (Perhaps a namespace issue at play here?) |
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
I have a few minor suggestions but all in all looks good.
If you have time to refactor install_dependency_symlinks I think that would be good to do here.
lib/spack/spack/package.py
Outdated
| # Note this is a feature required for external packages maintained by at least | ||
| # one site. | ||
| # | ||
| # TODO: Why is unused pkg one of the arguments? |
There was a problem hiding this comment.
This might be vestigial, IMO it's worth removing. In fact based on the test it looks like you can omit all arguments except for spec since you can get the prefix from Spec.prefix; that might be a spot where the people using this function need the two arguments though (since it's possible to choose any prefix). It's definitely safe to remove pkg though.
There was a problem hiding this comment.
Okay. I've asked Greg (@becker33) to confirm it's safe to make this change.
There was a problem hiding this comment.
pkg is in the place of self, I must have had the intention of making it a static or class method at some point in the past. I think the pkg argument should be renamed to self.
There was a problem hiding this comment.
It's not an instance method where self would make sense -- we should remove pkg.
There was a problem hiding this comment.
Hmm. It looks like I lost the changes I thought I'd made here.
There was a problem hiding this comment.
Removing pkg (or self) is not appropriate given it stands in for an instance method. So I added a package to illustrate its use and associated documentation.
|
@scheibelp I added comments based on @becker33's input (wrt use cases) and removed the assert per your feedback so this PR is ready for re-review. |
tldahlgren
force-pushed
the
tests/install_dependency_symlinks
branch
from
0e40e57 to
8a5d1d0
Compare
tgamblin
left a comment
tgamblin
left a comment
There was a problem hiding this comment.
LGTM modulo a couple small requests.
lib/spack/spack/package.py
Outdated
| # Note this is a feature required for external packages maintained by at least | ||
| # one site. | ||
| # | ||
| # TODO: Why is unused pkg one of the arguments? |
There was a problem hiding this comment.
It's not an instance method where self would make sense -- we should remove pkg.
|
@tgamblin I (roughly) restored a comment I thought I'd added; removed those you didn't want; and changed the test to conform to the expected use of the function. Is this version more acceptable? |
|
@scheibelp I removed the assert you requested. @tgamblin, @scheibelp, @becker33 This PR is ready to be reviewed again. |
|
@tgamblin @becker33 @scheibelp Are you okay with how I addressed your feedback and or comment changes? |
Made changes per Todd (tgamblin): - changed 'self' back to 'pkg'; - removed unnecessary module identifiers in the package.
|
@tgamblin Finished the tweaks we discussed w.r.t. restoring |
Removing the full module name reduced code coverage so restoring it here.
|
Interesting. Looks like code coverage didn't recognize the package install. So added an explicit @scheibelp @tgamblin This PR is ready for another review round. |
…upsream_develop * commit 'f7026a058b63f5a3109691e2c3871ee77c08f756': (1881 commits) Version 19.8.1 of PLASMA (spack#12299) new package: py-exodus (spack#12291) ncurses: fix pic and opt flags (spack#12272) pumi: new version 2.2.1 (spack#12282) tests: explain and test dependency flattening routines (spack#11993) graphviz package: add MacOS fixes and quartz support (spack#11128) Overhaul numpy package (spack#12170) mirrors: mirror config should use spack variable expansions (spack#9027) stacks: fix reference handling in env.write() (spack#12096) fltk: fix about variable types (spack#12292) Avoid sending empty reports to codecov (spack#12293) Packages/musl (spack#12288) c-blosc package: Add -std=gnu99 flag for gcc (spack#11959) Move new packages from tutorial to builtin (spack#12289) Balay/amrex 19.08 (spack#12287) openPMD-api: pre-load depend libs (spack#12279) Add version 19.8.0 of PLASMA (spack#12275) Add version 2.5.1 of MAGMA released today (spack#12274) ginkgo: add maintainers (spack#12273) new package: py-backports-tempfile (spack#12261) ... # Conflicts: # .travis.yml # var/spack/repos/builtin/packages/moab/package.py # var/spack/repos/builtin/packages/mofem-cephas/package.py # var/spack/repos/builtin/packages/mofem-fracture-module/package.py # var/spack/repos/builtin/packages/mofem-users-modules/package.py # var/spack/repos/builtin/packages/petsc/package.py
- Add comments to explain that `install_dependency_symlinks` and `flatten_dependencies` are actually used. - Add a test that exercises the routines.
4 participants
Added comments to explain that
install_dependency_symlinksandflatten_dependenciesare actually used.Added a test that exercises the routines.