move sbang to unpadded install tree root#19640
Conversation
|
@scottwittenburg @tgamblin @eugeneswalker This is the fix we discussed earlier today. Took some extra time to refactor for better separation of concerns between the store and backwards compatibility for configs (and get good test coverage). |
So here the padded install tree = With this PR, do we expect sbang to be at |
|
@eugeneswalker yes, that's correct. |
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
I have a few requests and questions
lib/spack/spack/util/path.py
Outdated
| padding to length | ||
|
|
||
| Assumes path does not have a trailing path separator""" | ||
| padding_length = length - len(path) - 1 # -1 for separator |
There was a problem hiding this comment.
(question) This logic handling the possibility of the padding length being only 1 larger seems new. Should it raise an exception? I assume if someone asks for padding, they need it.
There was a problem hiding this comment.
The reason it's new is because previously, the padding didn't have to start a new directory name, so we could patch foo/ to foos/. Now that we're storing the sbang at the top level in foo/bin/sbang, there is no way to pad foo to 4 characters. It's just not possible.
I'd be fine with having it raise an exception, but my guess is that it will A) never come up, and B) not be useful to users. I don't know of any use case for padding that actually depend on the exact number.
|
@scheibelp I've addressed all concerns except the last one, and that one may take more conversation. |
change how padding is specified
becker33
force-pushed
the
bugfix/padding-and-sbang
branch
from
0cb2b5e to
532be88
Compare
scheibelp
left a comment
scheibelp
left a comment
There was a problem hiding this comment.
I have two minor requests but this LGTM
@carsonwoods FYI this PR will generate a minor conflict with #11871 (like #19209, I don't think there will be a conceptual conflict)
lib/spack/spack/store.py
Outdated
|
|
||
|
|
||
| def parse_install_tree(): | ||
| """Parse old and new formats and return relevant values. |
There was a problem hiding this comment.
This docstring needs work. "relevant values" does not tell me anything about what it returns, as a naive reader has no idea what is relevant. This also doesn't tell me anything about the formats.
I'd expect there to be examples and descriptions of the formats here in the docstring, as well as description of the return type and what is in it. Use the google docstring format we use elsewhere, and add Arguments: and Returns: sections.
For testability and clarity, this should just take a dict as a parameter (I guess it should expect the result of config.get("config")) instead of calling config.get(). _store is the store-specific logic - let it call config.get and pass the result to parse_install_tree. This thing should just process a dict.
tgamblin
changed the title
5 participants
Fixes #19629.
Also changes how padding is specified.
Since #11598 sbang has been installed in the install_tree. This doesn't play nicely with install_tree padding, since sbang can't do its job if it's installed in a long path.
This PR changes the padding specification so padding can only be at the end of the string, and stores the install_tree root both with and without padding. The root without padding is stored as
spack.store.store.short_root.Now, sbang is installed to
spack.store.store.short_root/bin. This path should remain short enough for sbang to work.