Add cargo-spatial-codegen tool

[randomPoison]

This PR replaces the setup.sh helper script used in the project-example example with a new setup subcommand in the example application itself. This provides better cross-platform support for running the example, specifically it makes it easier to run the example on Windows.

Additional details:

• Added a new setup.rs module in project-example that replicates the logic in setup.sh.
• Added argument parsing for a new setup subcommand in argument_parsing.rs.
• Adds some additional logic in main() to handle the two different ways the example can be run (i.e. launching a worker or performing setup).
• Tweaked the logic for handling errors in the command line arguments to log directly to stderr instead of panicking).

Please let me know if there's anything I need to change!

[randomPoison]

Looks like the travis build failed, but I'm unclear on what the error is? It seems like the environment variable SPATIAL_OAUTH is missing, any idea why my change would have caused that?

[jamiebrynes7]

Ah that's because the PR is coming from a fork and the SPATIAL_OAUTH is a secret which Travis doesn't expose to PRs from forks. Will look into solutions for this 😄

[jamiebrynes7]

Looks like I'll have to manually run the CI suite locally and override the failing check when we go to merge. Will take a look at the code later 👍

[jamiebrynes7]

I like the change from sh -> cargo command as its very idiomatic and one less language/complexity to worry about.

However, I feel that such a tool would be better placed as a standalone binary in spatialos-sdk-tools since it can be made generic for all projects (given a set of input schema directories and an output directory).

Conflating the worker binary and the setup for the worker binary feels a little strange at first glance.

If you are up for it - I'd love it if you could refactor project-example/setup.rs into a standalone binary in the spatialos-sdk-tools crate.

In the future, it would probably be wrapped into a codegen tool which would compile the schema into the descriptor as well as generate Rust code.

[randomPoison]

I agree that splitting the setup script into its own is a good idea. I've been trying to setup my own project based on the example in this repo, and I'm finding that I need this setup script in order to compile the schema files. Splitting it off means that I can install it on its own, rather than having to copy-paste the script into my project.

[randomPoison]

btw I'm marking this as WIP, I want to do a cleanup pass and make sure errors are being handled properly before this gets merged.

[randomPoison]

I've cleaned up the error handling logic (i.e. not just panicking on every error) and removed the WIP marker.

[jamiebrynes7]

One small nit - but mostly LGTM

[jamiebrynes7]

Nice! LGTM - will run the code through "CI" later this evening 👍

[jamiebrynes7]

So running the command listed in the adjusted README gives me the following:

λ cargo run --bin setup -- spatialos-sdk/examples/project-example/spatialos/schema
   Compiling spatialos-sdk-tools v0.0.0 (C:\Workspace\personal\spatialos-sdk-rs\spatialos-sdk-tools)
    Finished dev [unoptimized + debuginfo] target(s) in 1.36s
     Running `target\debug\setup.exe spatialos-sdk/examples/project-example/spatialos/schema`
C:/Workspace\personal\spatialos-sdk-rs\dependencies\std-lib\improbable\standard_library.schema: Input file is not contained in any --schema_path. One --schema_path argument must be an exact prefix of the input file.
C:/Workspace\personal\spatialos-sdk-rs\dependencies\std-lib\improbable\vector3.schema: Input file is not contained in any --schema_path. One --schema_path argument must be an exact prefix of the input file.

Which looks like an error from the schema-compiler.

I think you've omitted the --schema-path argument seen here https://github.com/jamiebrynes7/spatialos-sdk-rs/blob/master/spatialos-sdk/examples/project-example/setup.sh#L15

EDIT: Actually you haven't. Not sure what's happening here 🤔

[randomPoison]

Hmmmmm, I was running into an issue similar to this before. It seems like schema-compiler requires --schema-path to be byte-for-byte identical with the leading part of any schemas you pass in. This can lead to it rejecting cases that should be valid. e.g. I ran into this on windows in cases where forward- and back-slashes got mixed up:

schema-compiler \
  --schema-path=C:\foo\bar\improbable/std-lib/schema \
  C:\foo\bar\improbable\std-lib\schema\whatever.schema

In that example --schema-path is logically a prefix of whatever.schema, however it mixes separators, which causes schema-compiler to reject the invocation.

You can actually see what is (I think) the cause in the example you posted:

C:/Workspace\personal\spatialos-sdk-rs\dependencies\std-lib\improbable\standard_library.schema

The first separator is / but the subsequent ones are \. I think the issue is in your SPATIAL_LIB_DIR variable, it probably defines the path with inconsistent separators.

Ultimately, this is an issue with shema-compiler in that it's rejecting perfectly valid paths. I can see if I can work around it for now by normalizing the paths before invoking the command.

[randomPoison]

I was able to reproduce the issue by using the --spatial-lib-dir argument to manually specify a path with mixed separators. I've added path normalization logic that fixes the issue in the cases that I was able to test. The logic is a bit ugly, but tbh the "right" solution is to fix schema_compiler to handle paths better. In the meantime, I've added a HACK comment clarifying exactly why the functionality is needed.

As an aside, there's a tracking issue for adding path normalization logic to std, but it hasn't landed on stable yet 😭

[randomPoison]

@jamiebrynes7 @davedissian I've updated this to add support for codegen in the setup script. setup can now run codegen, so there's only a single step users need to run in order to prepare their project (or the example). This should be ready for review/merge whenever you're ready 🙂

I've also deleted the generated_code.rs from the example and set it up to be ignored by git. The generated file should probably not be checked into git, since it will be generated by the user as part of the setup process.

[jamiebrynes7]

There's an argument that while codegen is under heavy development, having some example code checked in makes it a lot easier to review rather than t4 templates

[randomPoison]

I think that's a fair point, though I think it would make more sense to have small, controlled examples as part of the spatialos-sdk-code-generator crate, rather than using the example project for that purpose. That said, I'm happy to restore generated_code.rs if you'd prefer 🙂

[jamiebrynes7]

Can combine the above line with this block:

let mut command = Command::new(&schema_compiler_path)
	.arg(...)
	...
	.arg(...);

Also a line of whitespace between the last path constructor and the command declaration would help with readability :)

[randomPoison]

Unfortunately, I don't think I can combine these. On line 87 I conditionally add arguments to command (in a loop), which means I need the command bound to a variable. But if I try to bind the return value of arg() to a variable, I'll get a compiler error, e.g.

let mut command = Command::new(...)
    .arg(...)
    .arg(...);

gives the error:

error[E0716]: temporary value dropped while borrowed
  --> spatialos-sdk-tools\src/codegen/main.rs:79:23
   |
79 |     let mut command = Command::new(&schema_compiler_path)
   |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ creates a temporary which is freed while still in use
...
84 |         .arg("--load_all_schema_on_schema_path");
   |                                                 - temporary value is freed at the end of this statement
...
88 |         command.arg(&arg);
   |         ------- borrow later used here
   |
   = note: consider using a `let` binding to create a longer lived value

The only way to make this work is to bind the result of Command::new() to a variable, then call arg() on it in a chain.

---

As an aside, this is why I use the tap crate and avoid writing methods that return Self. There's no really good way to do method chaining by returning Self in Rust, and tap is the only robust solution I've found.

[jamiebrynes7]

Maybe codegen_out_path ?

[randomPoison]

Hrmmm, so I originally had it as something like that, but changed it to --codegen because I wanted it to more clearly indicate that specifying that flag was also enabling codegen. I can change it back to --codegen-out-path if you think that's better, I don't have super strong feelings about it either way. Though, if you think --codegen is fine as the CLI flag and you just want the variable to be codegen_out_path, I can also do that 😁

[jamiebrynes7]

Maybe descriptor_out_dir ?

[randomPoison]

output_dir is currently used for both schema.descriptor and bundle.json. Does it make sense to call it descriptor_out_dir when it's used for both? Or should those two be split into separate flags?

[randomPoison]

Do we even have to list all the schema files when invoking schema_compiler? If I'm reading the docs correctly, the --load_all_schema_on_schema_path flag that we're passing in is already telling it to load all the schema files. If we don't need to manually pass in any files, that would allow us to simplify the code further.

[randomPoison]

I'm going to close this PR in favor of working on a more robust cargo-spatial tool. A PR for that should be coming soon :3