Enable running some commands from subdirectories #2845
Conversation
Signed-off-by: itowlson <[email protected]>
crates/common/src/paths.rs
Outdated
| loop { | ||
| let candidate = inferred_dir.join(DEFAULT_MANIFEST_FILE); | ||
|
|
||
| if candidate.is_file() { | ||
| return Some((candidate, is_default)); | ||
| } | ||
|
|
||
| is_default = false; | ||
| let parent = inferred_dir.parent()?; | ||
|
|
||
| inferred_dir = parent.to_owned(); | ||
| } |
There was a problem hiding this comment.
Do we want to restrict the level of parent directories that we look for? or would that be unexpected behavior.
There was a problem hiding this comment.
The one heuristic I would maybe consider would be to stop the search if you see a .git directory. I can imagine edge cases where traversing up out of a repository could cause some pain. 🤷
There was a problem hiding this comment.
Okay, it now stops if it sees a .git directory, and I also changed the loop to top out at 20 iterations - this shouldn't stop normal use but it does provide belt and braces in case something weird is going on. The 20 is arbitrary and can be increased or decreased if people have opinions. "If" people have opinions, who am I kidding
src/commands/doctor.rs
Outdated
| let manifest_file = spin_common::paths::resolve_manifest_file_path(&self.app_source)?; | ||
| let (manifest_file, is_default) = | ||
| spin_common::paths::find_manifest_file_path(self.app_source.as_ref())?; |
There was a problem hiding this comment.
Perhaps we shouldn't apply this feature here given the good doctor's tendency to mutate manifests...
Alternatively - given that this code is already written - it could fail with a nice message e.g. "No 'spin.toml' in current directory; did you mean --from '../spin.toml'?"
src/commands/build.rs
Outdated
| spin_common::paths::find_manifest_file_path(self.app_source.as_ref())?; | ||
| if !is_default { | ||
| terminal::einfo!( | ||
| "Using 'spin.toml' from parent directory:", |
There was a problem hiding this comment.
parent directory
Technically could be any ancestor right? Not sure how best to express that... "containing directory"?
It might even be a nice easter egg to write out e.g. "great-great-grandparent directory" 🙂
There was a problem hiding this comment.
@lann I expected some high-quality bikeshedding on this one and I gotta say you have lived up to those expectations and more.
crates/common/src/paths.rs
Outdated
| loop { | ||
| let candidate = inferred_dir.join(DEFAULT_MANIFEST_FILE); | ||
|
|
||
| if candidate.is_file() { | ||
| return Some((candidate, is_default)); | ||
| } | ||
|
|
||
| is_default = false; | ||
| let parent = inferred_dir.parent()?; | ||
|
|
||
| inferred_dir = parent.to_owned(); | ||
| } |
There was a problem hiding this comment.
The one heuristic I would maybe consider would be to stop the search if you see a .git directory. I can imagine edge cases where traversing up out of a repository could cause some pain. 🤷
itowlson
force-pushed
the
search-up-dir-tree-for-spin-toml
branch
from
2caa522
to
c659715
Compare
|
I HOPE YOU ARE HAPPY LANN
|
|
SO HAPPY |
| return Some((candidate, 0)); | ||
| } | ||
|
|
||
| for distance in 1..20 { |
There was a problem hiding this comment.
20 seems a little bit high but no strong opinions. Maybe this is something we want to be configurable to users using env vars?
There was a problem hiding this comment.
Configuring it seems wildly excessive even by the standards of great-great-great-grandparent string code. This was randomly picked to be high enough that nobody will ever run into it but low enough that if we hit some kind of crazy infinite loop case then it will exit before taking too long. But happy to reduce it. I originally had it at 10. What am I bid?
(Price Is Right audience "higher! lower!" sound FX)
There was a problem hiding this comment.
@karthik2804 I'm going to punt on this since it's non-blocking - happy to tweak as a follow-up.
| } | ||
|
|
||
| for distance in 1..20 { | ||
| let inferred_dir = PathBuf::from("../".repeat(distance)); |
There was a problem hiding this comment.
FYI: https://doc.rust-lang.org/std/path/struct.Path.html#method.ancestors
e.g. for candidate in Path::new(".").ancestors().take(20)
There was a problem hiding this comment.
I don't think this does what I think you're suggesting. In my tests Path::new(".").ancestors() produced . then the empty string, then stops. Are you seeing something different? Maybe depends on OS?
(ETA: it would, of course, work with std::env::current_dir() as the starting point, but I intentionally chose to use relative instead of absolute paths because it showed the relationship more clearly, and tied in better to the "grandparent directory" explanation.)
There was a problem hiding this comment.
Oh sure enough. I glanced at the example in its docs, saw .. and mindlessly thought "that looks about right!"...
…spin.toml. Such precise, very great-grand. Signed-off-by: itowlson <[email protected]>
itowlson
force-pushed
the
search-up-dir-tree-for-spin-toml
branch
from
c659715
to
7d9e69d
Compare
|
oh wait no. I can't merge because Karthik isn't an approved reviewer. Aaaugh! |
Fixes #2828.
@karthik2804 you asked me a couple of days ago about something related to this, so I am not sure if you were already working on it - there was nothing in GitHub and I saw you were working on something much cooler - if you did have something under way then feel free to close this.
The experience for running from a subdirectory looks like this:
(Exception:
spin doctorwhere it integrates into an existing "this is the file you're partying on" notification.)That output is not currently centralised - I thought about making the
spin_commonfunction print it, butspin upcan't use that function anyway, and it would have requiredspin_commonto depend onterminal, so I passed on that for now. But we can do it if people prefer. I certainly don't much love the internal API (loose booleans, oh the self-loathing) but thought I'd get feedback before I determined how to clean it up.Also, the way I did it resulted in showing the absolute path (
/home/ivan/whatever/spin.toml) rather than the relative path (../spin.toml). The UX here seemed a little tricky - for one or two levels, the relative path is probably more helpful, but for more than that it just turns into a blur (../../../../../spin.toml). For now I went with the absolute path because I had it to hand, but realistically the commonest case is one level down, maybe two, so there's a strong case for doing it the other way. (Indeed, I'm thinking the terror and confusion of ../../../../../spin.toml would actually make it more noticeable that you were hitting some crazy random manifest you didn't intend.)This PR adds the "run from subdirectory" behaviour to:
We could add it to
spin addbut I think it might get confusing where the new component's directory got created. We do already allowspin add -f ../..though, so maybe I'm being overly cautious. Maybe not behaving like other commands is worse from a potential surprise POV.