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

Fix spin doctor spurious "missing Wasm file" if app is in another directory #1514

Merged
merged 1 commit into from
May 22, 2023

Conversation

itowlson
Copy link
Contributor

I ran into this because I was playing with spin doctor using spin doctor -f ~/testing/kvmadness and it reported that my Wasm file was missing. It looks like it currently probes for the file relative to the current directory rather than the app directory.

This fixes that, but it feels a bit verbose, like I am passing more stuff around more places than should be necessary. @lann I'd be grateful for any insight on how you envisaged things like this being structured. I saw that the WasmDiagnostic trait passes the PatientApp that includes the manifest file path, and originally used that to construct the absolute Wasm path in diagnose_wasm.
But it felt like that would end up getting repeated in multiple diagnostics so I moved it into the PatientWasm type. Anyway I don't want to end up spoiling the design on my first outing grin

@itowlson itowlson requested a review from lann May 18, 2023 01:26
@itowlson itowlson force-pushed the doctor-fix-wasm-not-in-current-dir branch from a6b578d to e181c58 Compare May 18, 2023 02:22
@lann
Copy link
Collaborator

lann commented May 18, 2023

This doesn't seem too bad to me, but another option to consider:

impl PatientWasm {
    // ...

    pub fn local_abs_path(&self) -> Option<PathBuf> {
        match self.source() {
            WasmSource::Local(path) => Some(self.app_dir.join(path)),
            _ => None,
        }
    }
}

Patient/PatientWasm feel like a good place to hang lots of this kind of helper method to ease diagnosis implementation.

@itowlson itowlson force-pushed the doctor-fix-wasm-not-in-current-dir branch from e181c58 to 9d13971 Compare May 19, 2023 01:22
@itowlson
Copy link
Contributor Author

Thanks @lann - I adopted that and it made for a cleaner diff I think. But it did make some of the "when do I use what" a bit fuzzier, so I ended up doing some renames anyway... Anyway ready for review now.

crates/doctor/src/wasm.rs Outdated Show resolved Hide resolved
Comment on lines 22 to 40
pub fn source(&self) -> WasmSource {
match &self.0.source {
pub fn rel_source(&self) -> WasmSource {
match &self.component.source {
RawModuleSource::FileReference(path) => WasmSource::Local(path),
_ => WasmSource::Other,
}
}
Copy link
Collaborator

@lann lann May 19, 2023

Choose a reason for hiding this comment

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

Maybe change this to source_path(&self) -> Option<&Path> and drop WasmSource entirely? I think that will be clear enough and we'll need to document these before stabilizing this interface anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

or maybe local_source_path and local_source_abs_path or something

@itowlson itowlson force-pushed the doctor-fix-wasm-not-in-current-dir branch from 9d13971 to c18e66d Compare May 21, 2023 21:49
@itowlson itowlson requested review from lann and rylev May 21, 2023 21:51
@rylev
Copy link
Collaborator

rylev commented May 22, 2023

Does this play well with the changes from #1503?

crates/doctor/src/wasm.rs Outdated Show resolved Hide resolved
@itowlson
Copy link
Contributor Author

@rylev Ugh, good catch. Probably not. It looks like those paths are resolved at the apping stage rather than the raw load stage, and the Wasm doctor wisely relies only on the raw manifest (because the apping stage requires a high degree of validity).

I guess I can bodge around it or rework the loader pipeline to more finely reflect load stages. The latter is my long term goal but I might try the former for now. sigh

@itowlson itowlson force-pushed the doctor-fix-wasm-not-in-current-dir branch from c18e66d to 1d09e13 Compare May 22, 2023 20:04
Signed-off-by: itowlson <ivan.towlson@fermyon.com>
@itowlson itowlson force-pushed the doctor-fix-wasm-not-in-current-dir branch from 1d09e13 to f50721e Compare May 22, 2023 20:30
@itowlson
Copy link
Contributor Author

@rylev updated for #1503

@itowlson itowlson requested a review from lann May 22, 2023 20:31
@itowlson itowlson merged commit 9791457 into fermyon:main May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants