feat: Windows support

[SecondFlight]

https://user-images.githubusercontent.com/6700184/136310269-c4313222-b6f0-4205-8ab2-5c31d0260cc3.mp4

Resolves #33.

This patch adds Windows support to rid, as well as an action to run make test on Windows.

[SecondFlight]

Not seeing builds on this PR, but here's one on my fork:

[thlorenz]

Not seeing builds on this PR, but here's one on my fork:

Not sure why it's not triggering CI. I have it setup that all PRs on master trigger it.

name: Build+Test

on:
  push:
    branches: [ master ]
  pull_request:
    branches: [ master ]

Oh well found the answer

There is an limitation for forked repo. Workflows do not run on private base repositories when you open a pull request from a forked repository. 🤦

I'll just check on your fork (maybe you can just give me access) that your PRs are passing CI and then can just merge them into master at which point they'll run here as well.

[SecondFlight]

I'll just check on your fork (maybe you can just give me access)

Sure thing, will do that in a bit.

[SecondFlight]

https://user-images.githubusercontent.com/6700184/136486966-32f2d31e-6d05-47fb-9e32-e03c6bc763c4.mp4

[SecondFlight]

Looks like you should already have access. Here's a link to the PR I opened over there to trigger CI runs: https://github.com/SecondFlight/rid/pull/4

Let me know if you can't access it.

[thlorenz]

Awesome pretty much LGTM at this point. Related to #39 I'd like to not split up these github action files, but would be willing to merge for now and have this fixed in a separate PR. Or could you take care of this as part of this PR? Just LMK what you prefer.

And thanks, I'm excited to see this run on windows as well 😄

[SecondFlight]

Thanks for the suggestion! It makes a lot of sense to me. I may as well fold the macOS stuff into this PR as well in that case.

I'm out right now but I'll try to get that in before tomorrow.

[thlorenz]

I may as well fold the macOS stuff into this PR as well in that case

That makes sense to me as well.

No worries, there is no rush at all with this, take your time.

[SecondFlight]

This should be good to merge now from my side. Passing build here: https://github.com/SecondFlight/rid/runs/3853411516

[thlorenz]

After updating the binding file related code + fixing the minor formatting NIT this looks good to merge. Awesome job!

[SecondFlight]

Okay, so cargo fmt is also broken because the rustfmt.toml files in the sub projects weren't parsing as valid toml. I searched for a good 15 minutes and couldn't seem to find anything documenting an import similar to how the root rustfmt.toml was being referenced. I also tried cargo +nightly fmt and cargo +nightly fmt -- --unstable-features, neither of which worked.

I "fixed" it by just making the rustfmt.toml files have identical content. Running cargo fmt at this point touched some other files as well, but I opted to only commit files also touched by this PR.

Would you mind checking this on your machine? I'd be happy to revert this change and just keep it locally, and if you know what I'm missing please let me know!

[thlorenz]

OK I'll have to look into this ... but it seems to be working for me (even though I get some error which I'll look into). It maybe a windows issue with links. I posted how this works on my machine below.

For now please just revert https://github.com/thlorenz/rid/pull/38/commits/f43a4bde641d07a870a65cbabc76845ef8521580 but keep the format fixes in. We'll figure the issue out later. Once we do I'll add a check to the build task to track this automatically in the future.

Thanks for your patience with the toml parsing part 😸

How it works on Macos

(from rid-macro-impl package)

➝  cargo fmt
error[internal]: left behind trailing whitespace
  --> ~/rid/rid/rid-macro-impl/src/render_rust/render_return_type.rs:74:74:1
   |
74 |
   | ^^^^^^^^
   |

error[internal]: left behind trailing whitespace
  --> ~/rid/rid/rid-macro-impl/src/render_rust/render_to_return_type.rs:55:55:58
   |
55 |             K::Composite(Composite::Vec, rust_type, _) =>
   |                                                          ^
   |

warning: rustfmt has failed to format. See previous 2 errors.

➝  git st
 M src/render_rust/render_return_type.rs
 M src/render_rust/render_to_return_type.rs
 M src/reply/mod.rs

From project root:

cargo fmt --all

with exact same results.

Obviously since I'm getting errors here this means that the fmt aspect isn't 100% clean at this point. I just always had vim auto-format for me, but seems some slipped through the cracks.

[SecondFlight]

Thanks for looking into this. Very strange. I'll revert https://github.com/thlorenz/rid/commit/f43a4bde641d07a870a65cbabc76845ef8521580 for now.

[SecondFlight]

When I run cargo fmt --all, I get this:

PS C:\Users\qbgee\Documents\Code\rid> cargo fmt --all
Could not parse TOML: expected a table key, found a period at line 1 column 1
PS C:\Users\qbgee\Documents\Code\rid> 

😕

[thlorenz]

That's odd .. does it indicate which toml file? At any rate this is good to merge for now right? We can figure out the format + toml issues later and fix in a separate PR.

[SecondFlight]

That's good with me. It doesn't say which file, but f43a4bd fixes it for me and those files all have a period at line 1 column 1, so I think that must be what it's referring to.

[thlorenz]

I'm getting the below warning when running the tests:

/Applications/Xcode.app/Contents/Developer/usr/bin/make test TEST=args_strings
pub get
The top level `pub` command is deprecated. Use `dart pub` instead.

It's not happening in CI, so it might be due to me being on the beta channel:

➝  flutter --version
Flutter 2.6.0-5.2.pre • channel beta • https://github.com/flutter/flutter
Framework • revision 400608f101 (4 weeks ago) • 2021-09-15 15:50:26 -0700
Engine • revision 1d521d89d8
Tools • Dart 2.15.0 (build 2.15.0-82.2.beta)

It's not a blocker for this PR, so we only should address it now if you know right away what the cause could be.

Otherwise we'll take care of it later.

[SecondFlight]

Prior to #35, make test would run pub get and fail if pub didn't exist. Now it prefers pub get but falls back to flutter pub get if pub doesn't exist. #35 added flutter pub as a fallback, and this PR adds dart pub as an additional fallback.

In light of that deprecation, I'd suggest preferring dart pub and falling back to flutter pub.

[SecondFlight]

Checks here: https://github.com/SecondFlight/rid/pull/4/checks?check_run_id=3877643154