LLNL / uberenv

Automates using spack to build and deploy software
Other
25 stars 9 forks source link

Unify spack command into separate function #95

Closed chapman39 closed 1 year ago

chapman39 commented 1 year ago

In preparation for moving to spack environments mentioned in issue https://github.com/LLNL/uberenv/issues/93 and was originally from PR https://github.com/LLNL/uberenv/pull/75.

cyrush commented 1 year ago

Looks good @chapman39

Suggestion for the future: Instead of calling the shell execute (sexe) command with a first argument of self.spack_exe_path would it make sense to have a function called spack_command where we pass in the command and options more explicitly?

E.g. instead of

sexe('{0} find -p /{1}, self.spack_exe_path(), pkg_hash)

consider:

spack_command("find", "-p /{0}".format(pkg_hash))

This might make the code's intent more obvious to readers.

It might also work well with environments since we can add an optional third parameter that passes in the environment and/or uses the default one.

Note on this: There are cases when we capture output from spack command and cases where we simply run the spack command.

So if we wrap these into a higher level function, we also need to make sure to wrap the ret_output argument semantics as well.

Handling those cases was one reason why I didn't refactor that far when I took my pass at this, but this is a good idea to tackle in the future -- would simplify the code big time.