Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unify spack command into separate function #95

Merged
merged 4 commits into from
Dec 12, 2022

Conversation

chapman39
Copy link
Collaborator

@chapman39 chapman39 commented Dec 8, 2022

In preparation for moving to spack environments mentioned in issue #93 and was originally from PR #75.

  • Instead of writing the spack executable path completely each time, calls spack_exe_path()
  • Prints spack version in show_info()
  • Additionally prints hash on error in find_spack_pkg_path_from_hash()
  • Additionally prints spec on error in find_spack_pkg_path()
  • Fixes bug where sometimes the spack package hash has a leading space, which causes it not to be found in find_spack_pkg_path_from_hash()

@chapman39 chapman39 added the enhancement New feature or request label Dec 8, 2022
@chapman39 chapman39 self-assigned this Dec 8, 2022
uberenv.py Show resolved Hide resolved
Copy link
Member

@cyrush cyrush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Copy link
Member

@kennyweiss kennyweiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cyrush
Copy link
Member

cyrush commented Dec 10, 2022

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.

@chapman39 chapman39 merged commit 3500404 into main Dec 12, 2022
@chapman39 chapman39 deleted the feature/chapman39/unify_spack_cmd branch December 12, 2022 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants