Add Windows implementations. by kazatsuyu · Pull Request #45 · BrainiumLLC/cargo-mobile

kazatsuyu · 2021-10-23T04:10:21Z

The following points should be noted:

cargo mobile update has not been tested, but it will probably fail on Windows because the running binary cannot be overwritten.
Use hard links for libraries under jniLibs since symbolic links cannot be created, and simply copy them if they fail.
Instead of symbolic links for assets directory, copy them at Gradle build time.

…ce on Android

kazatsuyu · 2021-11-02T12:57:54Z

Can someone please review it?

…mobile into feature/windows

zeerooth · 2021-11-03T17:48:10Z

oh god, I'm so sorry, I wanted to enable the windows CI on your branch, but I accidentally pushed some changes from my other branch, I'll try to have that sorted out

src/os/macos/mod.rs

src/os/windows/mod.rs

templates/platforms/android-studio/app/build.gradle.kts.hbs

zeerooth

I think that generally this PR is very sound and well-made, good job!

But, before merging there are a couple of things to sort out:

We need to resolve the issue with symlinking the assets directory, as discussed here
One it's done I'd be nice to test the basic commands like cargo mobile init, cargo mobile build, cargo mobile open etc. across all supported platforms, because there are changes in this PR that indirectly affect Linux and Mac as well
Update the README (add Windows to supported platforms and mention any Windows' specific caveats like cargo mobile update failing)

On Windows, Path::join may makes invalid UNC path. It was fixed on nightly (rust-lang/rust#89270), but not yet on stable.

kazatsuyu · 2021-11-05T13:16:31Z

I think I've solved the problem of symbolic links.

In addition, I fixed a path problem in stable that I missed using nightly.

I've confirmed that the commands work on Windows except cargo update, but I haven't checked if there are no problems on Linux and macOS. (Of course, it would be better to have someone other than me test it on Windows as well.)

This reverts commit 76f04bf.

This reverts commit 469c7ce.

This reverts commit 437f29e.

src/env.rs

zeerooth · 2021-11-29T22:59:12Z

Once again, thank you so much for your contribution!
I think that now this PR is ready to be merged - I've briefly tested the latest version on Linux and Mac, updated the README and approved it, but since the changes here are quite massive, I'd like someone else to take a look and approve as well.

@francesca64 may I kindly ask you to take a look?

zeerooth

I wanted to merge this but I encountered a weird behaviour when testing on this branch now...
When I run cargo mobile init for the first time the project is created successfully. However, if I do it the second time, I get the following error:

ln: failed to create symbolic link '/home/radek/code/cargo-mobile-sandbox/a/gen/android/a/app/src/main/assets': No such file or directory
error: Asset dir couldn't be symlinked into Android project
    Failed to create a symbolic link from "../../../../../../assets" to directory "/home/radek/code/cargo-mobile-sandbox/a/gen/android/a/app/src/main/" (file and directory clobbering enabled): `ln` command failed: Command
    "ln -n -s -f ../../../../../../assets /home/radek/code/cargo-mobile-sandbox/a/gen/android/a/app/src/main/assets" didn't complete successfully, exiting with code 1.

and the gen/android/a/app/src/main directory appears to be missing.
When I then run cargo mobile init again, the project is initialized successfully.
Basically, this behaviour alternates and it needs to be fixed.

New changes are needed

francesca64

Thanks for making such a massive contribution!

(and thanks to @zeerooth for thoroughly reviewing it!)

francesca64 · 2021-12-15T19:56:49Z

src/os/windows/mod.rs

+                &mut len as _,
+            )
+        }?;
+        return Ok(command);


Suggested change

return Ok(command);

Ok(command)

francesca64 · 2021-12-15T20:12:46Z

src/util/path.rs

+            }
+            Component::CurDir => {}
+            Component::ParentDir => {
+                if let Some(_) = buf.last() {


Suggested change

if let Some(_) = buf.last() {

if let buf.last().is_some() {

francesca64 · 2021-12-15T20:16:29Z

src/os/windows/mod.rs

+// %~dp0 will expand to C:\Users\MyHome\foo in code.cmd, which is completely broken.
+// Running it through powershell.exe does not have this problem.
+pub fn code_command() -> bossy::Command {
+    bossy::Command::impure("powershell.exe").with_args(&["-Command", "code"])


Suggested change

bossy::Command::impure("powershell.exe").with_args(&["-Command", "code"])

bossy::Command::impure_parse("powershell.exe -Command code")

francesca64 · 2021-12-15T20:36:09Z

src/os/windows/mod.rs

+
+pub fn command_path(name: &str) -> bossy::Result<bossy::Output> {
+    bossy::Command::impure("where.exe")
+        .add_arg(name)


Suggested change

.add_arg(name)

.with_arg(name)

francesca64 · 2021-12-15T20:39:26Z

src/os/windows/mod.rs

+        .expect("failed to getAndroid Studio uninstaller's parent path")
+        .join(STUDIO_EXE_PATH);
+    bossy::Command::impure(application_path)
+        .add_arg(


Suggested change

.add_arg(

.with_arg(

francesca64 · 2021-12-15T20:47:57Z

src/os/windows/ln.rs

+        )
+    });
+    if handle.is_invalid() {
+        return Err(runtime::Error::from_win32());


You can drop the return keyword and use else to return the Ok(()) below.

francesca64 · 2021-12-15T20:48:57Z

src/os/windows/mod.rs

+            err => Err(err),
+        })?;
+        let argv: Vec<_> = NativeArgv::new(&editor_command).into();
+


Suggested change

Extraneous newline

francesca64 · 2021-12-15T20:50:10Z

src/os/windows/mod.rs

+
+impl Application {
+    pub fn detect_editor() -> Result<Self, DetectEditorError> {
+        let editor_command = Self::detect_associated_command(RUST_EXT).or_else(|e| match e {


Rename e to err and the current err arm to _

francesca64 · 2021-12-15T20:54:06Z

src/os/windows/mod.rs

+            .map(|arg| Self::replace_command_arg(arg, &path.as_ref().as_os_str()))
+            .collect::<Vec<_>>();
+        bossy::Command::impure(&self.argv[0])
+            .add_args(&args)


Suggested change

.add_args(&args)

.with_args(&args)

francesca64 · 2021-12-15T21:00:55Z

src/os/windows/mod.rs

+
+#[derive(Debug, Error)]
+pub enum DetectEditorError {
+    #[error("No default editor is set: AssocQueryStringW for \".rs\" and \".txt\" both failed")]


Error text needs to be a high-level user-friendly description that doesn't i.e. mention function names (while still being unambiguous enough that a developer can find the source)

amrbashir · 2022-08-16T20:05:10Z

If there is any work needed for this PR to be merged, I'd be glad to help (ofc, If the author of this PR gives me permissions to, or if he is inactive, I could create a new PR that is build on top of this PR and keep his commits in the new PR)

kazatsuyu and others added 10 commits October 23, 2021 12:54

Add Windows implementations.

d9ad23f

Fix several environment variable error.

cd3b210

Fix generating build.gradle.kts bug.

a44d566

add egui example

99c2c05

fix egui template on desktop

549addd

Merge branch 'master' into egui-example

3b2e730

bump egui_wgpu_backend to 0.14 in egui template

ff8916e

egui template: handle Resumed/Suspended events, start without a surfa…

a394f85

…ce on Android

enable fullscreen in android by default

a6783d3

Merge branch 'master' into feature/windows

18b8526

zeerooth added 2 commits November 3, 2021 18:39

Merge branch 'feature/windows' of https://github.com/kazatsuyu/cargo-…

0162ea4

…mobile into feature/windows

enable windows in CI

dda5123

revert the accidental commits from another PR

4e7e2d4

zeerooth suggested changes Nov 3, 2021

View reviewed changes

src/os/macos/mod.rs Outdated Show resolved Hide resolved

src/os/windows/mod.rs Outdated Show resolved Hide resolved

zeerooth reviewed Nov 3, 2021

View reviewed changes

templates/platforms/android-studio/app/build.gradle.kts.hbs Outdated Show resolved Hide resolved

zeerooth added host: Windows type: enhancement Wouldn't this be the coolest? labels Nov 3, 2021

kazatsuyu added 2 commits November 4, 2021 20:58

Fix build errors.

c1ded18

Merge branch 'master' into feature/windows

8c8486d

kazatsuyu requested a review from zeerooth November 4, 2021 12:16

Fix macos build errors.

0f40b7b

zeerooth reviewed Nov 5, 2021

View reviewed changes

kazatsuyu added 3 commits November 5, 2021 20:17

Avoid Windows invalid UNC path.

13831f6

On Windows, Path::join may makes invalid UNC path. It was fixed on nightly (rust-lang/rust#89270), but not yet on stable.

Link assets directory on build time

437f29e

Fix path errors.

e366a81

kazatsuyu requested a review from zeerooth November 5, 2021 13:18

Fix symlink target path mistake.

469c7ce

kazatsuyu added 11 commits November 6, 2021 13:03

Refix symlink path.

76f04bf

Revert "Refix symlink path."

6f4ae53

This reverts commit 76f04bf.

Revert "Fix symlink target path mistake."

c06a750

This reverts commit 469c7ce.

Revert "Link assets directory on build time"

cafeca5

This reverts commit 437f29e.

Symlink for windows.

266f2d0

Add error message for unpriviledged Windows.

d0dab09

Add missing trailing newline in Cargo.toml

1280bd3

Remove syncAssets task.

7037e0c

Remove one level of redundant assets

10d7cc8

Refactoring a little

fa4862a

Use Clobber::FileOrDirectory instead of Clobber::File

879f42f

zeerooth approved these changes Nov 15, 2021

View reviewed changes

Merge branch 'master' into feature/windows

e45a1fc

zeerooth suggested changes Nov 15, 2021

View reviewed changes

src/env.rs Outdated Show resolved Hide resolved

zeerooth and others added 6 commits November 16, 2021 00:51

add simple prefix_path tests

a64450a

Merge branch 'master' into feature/windows

2cdd8d3

Use original target path in error message.

0cd4a6a

Use std symlink function instead of WinAPI

2a1de52

Add Windows specific Env struct.

a4bd539

update README.md with information about Windows support

398db8c

zeerooth previously approved these changes Nov 29, 2021

View reviewed changes

zeerooth requested a review from francesca64 December 2, 2021 23:10

resolve conflicts

4ce2044

zeerooth reviewed Dec 14, 2021

View reviewed changes

francesca64 suggested changes Dec 15, 2021

View reviewed changes

amrbashir mentioned this pull request Aug 16, 2022

fix(android/run): use manual upper camel case for gradle flavor task #106

Closed

	bossy::Command::impure("powershell.exe").with_args(&["-Command", "code"])
	bossy::Command::impure_parse("powershell.exe -Command code")

Conversation

kazatsuyu commented Oct 23, 2021

Uh oh!

kazatsuyu commented Nov 2, 2021

Uh oh!

zeerooth commented Nov 3, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zeerooth left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kazatsuyu commented Nov 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

zeerooth commented Nov 29, 2021

Uh oh!

zeerooth left a comment

Choose a reason for hiding this comment

Uh oh!

francesca64 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amrbashir commented Aug 16, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zeerooth left a comment •

edited

Loading

kazatsuyu commented Nov 5, 2021 •

edited

Loading