feat: add workdir for component build command

[mooori]

Fixes #406

If workdir is not relative an error is returned, assuming it's best to abort
on encountering an invalid value in the configuration. Alternatively, an invalid
workdir could be ignored and a warning be logged.

Specifying a workdir which is not a subdirectory of src is currently
allowed, for example ../other-app. I'm assuming that's okay since
users should inspect spin.toml anyway before running spin (e.g. to check if
they really want to execute the build command).

Example usage

mkdir examples/foo
cp examples/http-rust/spin.toml examples/foo

In examples/foo/spin.toml updated resp. add the following:

source = "../http-rust/target/wasm32-wasi/release/spinhelloworld.wasm"

# under [component.build] add
workdir = "../http-rust"

After building spin with these changes, it is possible to:

cd examples/foo && spin build

Signed-off-by: Moritz moritz.zielke@gmail.com

[lann]

This won't catch --workdir=... Perhaps it should be:

Suggested change

	if !src.as_ref().starts_with(workdir.as_path()) {
	if b.workdir.is_some() {

or just print the workdir unconditionally.

[mooori]

The workdir returned by construct_workdir is absolutized, so it shouldn't contain dots. But that's implicit and your suggestion definitely makes the intent clearer.

[lann]

Right, and workdir is also absolutized at this point, so --workdir=.. would always resolve to a prefix of src.

[mooori]

The test which failed on windows is refactored, it should be independent of the OS now. I can't test it on windows myself, unfortunately, since I don't have a windows machine. Waiting on CI.

[lann]

Thanks for the PR!

[mooori]

The test which failed on windows is refactored, it should be independent of the OS now. I can't test it on windows myself, unfortunately, since I don't have a windows machine.

That didn't work. These assertions seem to be failing in windows-latest CI:
https://github.com/mooori/spin/blob/dd31cabe1a715e1ced8f8aca48b33e3d7d2630b9/crates/build/src/lib.rs#L120-L121

Maybe use cfg!(windows) to supply another value for expected when running on windows?

[lann]

Ah sorry, you probably can't see the CI logs:

thread 'tests::test_construct_workdir' panicked at 'assertion failed: `(left == right)`
  left: `Err(StripPrefixError(()))`,
 right: `Ok("")`: "D:\\home\\alice\\app\\foo\\bar" != "/home/alice/app/foo/bar"', crates\build\src\lib.rs:109:13

Looks like its a Windows \ vs *nix / path separator problem. From a quick browse of the docs it is surprisingly difficult to implement your tests in a cross-platform manner. I think the most straightforward solution might be something like this awful hack: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=09f6039526dda72c289dcdd0b5bb9655 Actually, I'm not sure this works either since you would still need the D: prefix... I wouldn't hard-code that either, since D: will change between computers. It might just require more logic in assert_workdir...GAH!

Additionally, as I was looking at the docs for Path, I noticed that Path::is_absolute has some surprising behavior on Windows and you might prefer Path::has_root.

[mooori]

Could the following work cross-platform?

• Add an integration test for spin build specifying a dummy build command like e.g.

command = "touch foo" # quick and doesn't produce large artifacts

• The test's spin.toml specifies a workdir and it's verified that the output of the build comand ends up in the desired directory.

Then we would be testing the behavior of construct_workdir via the public API.

If this works, I think test_construct_workdir could be removed. It tests a private function, anyway.

Additionally, as I was looking at the docs for Path, I noticed that Path::is_absolute has some surprising behavior on Windows and you might prefer Path::has_root.

I'm going to replace Path::is_relative with Path::is_absolute Path::has_root.

[lann]

Add an integration test for spin build specifying a dummy build command like e.g.

👍 Sounds like a good plan.

I'm going to replace Path::is_relative with Path::is_absolute.

Those are just exact opposites. Path::has_root is subtly different, and only on Windows:

• On Windows, a path is absolute if it has a prefix and starts with the root: c:\windows is absolute, while c:temp and \temp are not.

I don't think it makes a ton of difference either way; I think those two relative path forms are uncommon on Windows anyway.

[mooori]

Add an integration test for spin build specifying a dummy build command like e.g.

+1 Sounds like a good plan.

I'm going to give it a try! Maybe rather in a separate PR?

I'm going to replace Path::is_relative with Path::is_absolute.

Oops sorry, I meant to say I'm going to replace it with Path::has_root.

[lann]

I'm going to give it a try! Maybe rather in a separate PR?

Do you mean replacing this one or after this one? We don't want to merge a PR that breaks CI.

[mooori]

The test_construct_workdir is removed and Path::has_root is used.

Assuming CI now passes on Windows too, the idea was to merge this PR as is and then open a new one to add the integration test of spin build. Possibly a tracking issue could be created. What do you think?

[lann]

Sounds good; thanks for the contribution!