feat: Windows support #38

SecondFlight · 2021-10-07T02:36:32Z

MgGJq4f1SM.mp4

Resolves #33.

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

SecondFlight · 2021-10-07T02:37:18Z

Makefile.variable

+		ifeq (, $(shell $(TEST_FOR_DART)))
+			$(error $(ERR_PUB_NOT_FOUND))
+		else
+			PUB_PREFIX := dart


Since rid doesn't actually depend on Flutter, I added an extra check here for dart pub.

examples/dart/clock/rid_build.rs

SecondFlight · 2021-10-07T02:39:40Z

rid-build/src/ffigen/run_ffigen.rs

+        if cfg!(target_os = "windows") {
+            cmd.args(&["-c", FFIGEN_RUNNER]);
+        }
+


I have no idea why this didn't work on Windows. I tried it on two different machines. The original code caused the build script to fail with a cryptic error - something like "the system could not find the file specified" with no stack trace.

rid-build/src/lib.rs

SecondFlight · 2021-10-07T02:40:41Z

rid-build/src/project.rs

+            binding_file: None,
+            swift_plugin_file: None,
+        }
+    }


I don't know what this does but the other OSes have an entry here and I didn't want Windows to feel left out.

Only iOS + macOS will have swift_plugin_file entry. All should have a binding_file.

Hmm... Android and Linux don't have one either. Should this follow the same format as above?

I think you'd follow closer to Linux/Android for Windows in this case as no Swift is used there either.

I apologize, I'm confused. Linux and android don't have a binding_file. Should I leave this out on Windows as well?

If no binding.h file needs to be copied anywhere to make this work on windows then yes you should just leave it out.

Okay. This isn't something I encountered on Windows, so either it's not needed or it was somehow already where it needed to be. I'll definitely be looking out for this though, in case there was something weird that I missed!

SecondFlight · 2021-10-07T02:42:06Z

tests/dart/apps/Makefile

@@ -3,15 +3,13 @@ include ../../../Makefile.variable
 test-all: 
 	$(MAKE) test TEST=todo

-test: ./.dart_tool/
+test:
+	$(PUB_PREFIX) pub get


I replaced the ./.dart_tool/ step in each Makefile, first with $(MAKE) ./.dart_tool/ and then with a direct call to pub get. At first it wasn't running at all for me. With the first change it would run sometimes, but I had issues with the package resolution being stale and causing errors. Running it every time fixed the issue, and it's hardly slower on my machine. Pub exits quickly when it has nothing to update.

Not sure about this one, what this is basically saying is:

This is how to produce the ./.dart_tool/ directory if it is not there

./.dart_tool/: $(PUB_PREFIX) pub get

The test task needs the ./.dart_tool/ directory to be present (otherwise it'll run $(PUB_PREFIX) pub get since we told it above how to produce that directory.

test: ./.dart_tool/ ....

With your change we now always run pub get when starting the tests even if it is not necessary if we ran that step before (indicated by the presence of ./.dart_tool/).

It could be that on windows that directory has a different name or the info of installed packages is stored elsewhere.
This is what it looks like for me:

➝ ls ./.dart_tool/ package_config.json pub version package_config_subset test

`

Thanks for the explanation.

With your change we now always run pub get when starting the tests even if it is not necessary if we ran that step before (indicated by the presence of ./.dart_tool/).

That matches what I saw. My experience was that pub would get in a state where ./.dart_tool/ was present but the contents were stale. I had a specific file-not-found error that I fixed with this change, where pub definitely needed to run but it wasn't being run.

I can change this back but it wasn't working for me.

Ah I understand. I think making things less brittle should be preferred over optimization, even though you could just delete ./.dart_tool/ or run flutter pub get manually in order to fix the state.

Another reason I used this approach was to be able to work offline.
However it's annoying if things break for odd reasons so I'd opt for your change in this case.

Maybe the best case then is to run pub but soft-fail if it doesn't work? Though maybe that's what it's already doing, I'm still not super strong with make. I'll check this out when I update the PR.

I just checked in on this. On my system, pub get doesn't seem to fail when you're offline, so I don't think anything needs to change.

SecondFlight · 2021-10-07T02:44:39Z

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

SecondFlight · 2021-10-08T02:04:03Z

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

Sure thing, will do that in a bit.

SecondFlight · 2021-10-08T02:07:36Z

RbJopDLvRc.mp4

SecondFlight · 2021-10-08T02:11:19Z

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 · 2021-10-10T15:06:00Z

rid-template-flutter/setup.sh

@@ -18,7 +18,7 @@ cp $TEMPLATE_ROOT/rust/Cargo.toml $APP_ROOT/Cargo.toml
 perl -pi -w -e "s/<package>/$APP_NAME/;" $APP_ROOT/Cargo.toml

 # Plugin
-flutter create --platforms=android,ios,macos --template=plugin $APP_ROOT/plugin
+flutter create --platforms=android,ios,macos,linux,windows --template=plugin $APP_ROOT/plugin


What happens here if I'm not on windows? Will it just create the folders and work even if I don't have the respective binaries to support windows on flutter? Or would it break?
This is not a huge deal either way as the CLI tool will take care of selecting the proper commands given the platforms a user wants to support, however I want to avoid breaking devs coming to the project and setting up an example.

A good question. This tells flutter what platforms you want your code to work on, not what platform you're currently on. If I create a repo like this on Windows and you pull it down on macOS then you will be able to build it as a desktop app, whereas if I didn't include macOS then you wouldn't.

thlorenz · 2021-10-10T15:08:18Z

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 · 2021-10-10T15:47:42Z

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 · 2021-10-10T16:00:45Z

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 · 2021-10-10T22:27:39Z

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

thlorenz · 2021-10-11T01:36:09Z

rid-build/src/ffigen/run_ffigen.rs

+        // expected.
+
+        let mut cmd = Command::new(
+            if cfg!(target_os = "windows") {"powershell"} else {FFIGEN_RUNNER}


Formatting looks odd here, does your editor/IDE not autoformat? If it was it should've picked up the settings in rustfmt.toml and changed it to something like:

let mut cmd = Command::new(if cfg!(target_os = "windows") { "powershell" } else { FFIGEN_RUNNER });

I suppose running rustfmt on the file should do the trick as well.

Okay, weird. ctrl+shift+p -> format document looks like its doing something but it definitely isn't. I'll use the regular command line rustfmt for now.

thlorenz · 2021-10-11T01:38:28Z

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

SecondFlight · 2021-10-11T02:34:10Z

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 · 2021-10-11T23:35:33Z

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 f43a4bd 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 · 2021-10-12T00:51:15Z

Thanks for looking into this. Very strange. I'll revert f43a4bd for now.

SecondFlight · 2021-10-12T00:53:57Z

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 · 2021-10-12T12:58:14Z

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 · 2021-10-12T13:35:47Z

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 · 2021-10-13T01:40:43Z

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 · 2021-10-13T02:07:03Z

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 · 2021-10-13T02:10:22Z

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

SecondFlight and others added 20 commits October 5, 2021 10:38

feat: root makefile windows support

77cf4ee

feat: fall back to flutter pub if pub is missing

b882822

fix: broken pub finder

ac06693

feat: make sub tests cross-platform until rid-build

2b7f0a7

chore: refactor Makefile.variable

8f24cf6

fix: set test variable without space on Windows

be2cb07

fix: address review comments

5c1839d

Merge branch 'fix/local-test-failure' into feat/windows-support

ca766b6

fix: prefer dart pub

8faf38b

fix: hack - run dart under powershell on Windows

e63334c

revert: dart pub does not need to be preferred over flutter pub

8e955fa

fix: run pub as explicit make step

4f02665

feat: fall back to dart pub flutter pub not found

b80266c

fix: Windows DLLs don't have lib prefix

ea5024a

feat: functioning make test on Windows

349d8cd

Merge remote-tracking branch 'upstream/master' into feat/windows-support

1cba612

fix: prevent staleness by always running pub get

68d796b

fix: lib_name uses crate_name when not in test

63495a9

feat: add Windows test action

3d2c1e6

Merge remote-tracking branch 'origin/master' into feat/windows-support

eb57c04

SecondFlight added enhancement New feature or request windows Issues/PRs related to using rid on Windows labels Oct 7, 2021

SecondFlight requested a review from thlorenz October 7, 2021 02:36

SecondFlight commented Oct 7, 2021

View reviewed changes

examples/dart/clock/rid_build.rs Show resolved Hide resolved

SecondFlight commented Oct 7, 2021

View reviewed changes

rid-build/src/lib.rs Outdated Show resolved Hide resolved

SecondFlight commented Oct 7, 2021

View reviewed changes

thlorenz reviewed Oct 10, 2021

View reviewed changes

SecondFlight mentioned this pull request Oct 10, 2021

feat: macOS build+test action #39

Closed

SecondFlight added 4 commits October 10, 2021 16:49

feat: unify build+test step and add macOS support

222ba85

fix: use correct matrix values

c90f8f5

fix: add LLVM step for Ubuntu

eab991a

fix: only run LLVM install step on Ubuntu

a6e32ed

thlorenz reviewed Oct 11, 2021

View reviewed changes

SecondFlight added 2 commits October 10, 2021 22:27

fix: rustfmt parse failure

f43a4bd

chore: cargo fmt

0157d72

revert: undo f43a4bd

c665650

chore: pub -> dart pub

b38b554

thlorenz merged commit 48e11d9 into thlorenz:master Oct 13, 2021

thlorenz mentioned this pull request Oct 16, 2021

bindgen doesn't generate full path for dart_ffi.DynamicLibrary.open in rid-api.dart #43

Open

feat: Windows support #38

feat: Windows support #38

Conversation

SecondFlight commented Oct 7, 2021 • edited Loading

SecondFlight Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

SecondFlight Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

SecondFlight Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SecondFlight Oct 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SecondFlight Oct 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SecondFlight commented Oct 7, 2021

SecondFlight commented Oct 8, 2021

SecondFlight commented Oct 8, 2021

SecondFlight commented Oct 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thlorenz commented Oct 10, 2021

SecondFlight commented Oct 10, 2021 • edited Loading

thlorenz commented Oct 10, 2021 • edited Loading

SecondFlight commented Oct 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thlorenz commented Oct 11, 2021 • edited Loading

SecondFlight commented Oct 11, 2021 • edited Loading

thlorenz commented Oct 11, 2021 • edited Loading

How it works on Macos

SecondFlight commented Oct 12, 2021

SecondFlight commented Oct 12, 2021

thlorenz commented Oct 12, 2021

SecondFlight commented Oct 12, 2021

thlorenz commented Oct 13, 2021 • edited Loading

SecondFlight commented Oct 13, 2021 • edited Loading

SecondFlight commented Oct 13, 2021

SecondFlight commented Oct 7, 2021 •

edited

Loading

SecondFlight Oct 7, 2021 •

edited

Loading

SecondFlight Oct 7, 2021 •

edited

Loading

SecondFlight Oct 7, 2021 •

edited

Loading

SecondFlight Oct 11, 2021 •

edited

Loading

SecondFlight Oct 8, 2021 •

edited

Loading

SecondFlight commented Oct 10, 2021 •

edited

Loading

thlorenz commented Oct 10, 2021 •

edited

Loading

thlorenz commented Oct 11, 2021 •

edited

Loading

SecondFlight commented Oct 11, 2021 •

edited

Loading

thlorenz commented Oct 11, 2021 •

edited

Loading

thlorenz commented Oct 13, 2021 •

edited

Loading

SecondFlight commented Oct 13, 2021 •

edited

Loading