Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sh permission #1842

Merged
merged 29 commits into from
Apr 4, 2024
Merged

Fix sh permission #1842

merged 29 commits into from
Apr 4, 2024

Conversation

canxin121
Copy link
Contributor

Changes

close #1840

Modify frb_codegen\assets\integration_template\rust_builder\macos\REPLACE_ME_RUST_CRATE_NAME.podspec and frb_codegen\assets\integration_ template\rust_builder\ios\REPLACE_ME_RUST_CRATE_NAME.podspec in the
script_phases, add Build Rust library before the
`
{
:name => 'Change Permissions for Scripts', :script => 'chmod'
:script => 'chmod a+x "$PODS_TARGET_SRCROOT/... /cargokit/run_build_tool.sh" && chmod a+x "$PODS_TARGET_SRCROOT/... /cargokit/build_pod.sh"', :execution_position = >:execution_position
:execution_position => :before_compile
},
'
to ensure that the sh script has executable permissions.

Checklist

  • [ √ ] An issue to be fixed by this PR is listed above.
  • New tests are added to ensure new features are working. Please refer to this page to see how to add a test.
  • ./frb_internal precommit --mode slow (or fast) is run (it internal runs code generator, does auto formatting, etc).
  • If this PR adds/changes features, documentations (in the ./website folder) are updated.
  • CI is passing. Please refer to this page to see how to solve a failed CI.

Copy link

welcome bot commented Mar 28, 2024

Hi! Thanks for opening this pull request! 😄

@canxin121
Copy link
Contributor Author

As verified by MacOS Runner using Github Actions, this pr works fine and resolves the previous permissions issue.
Action

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 28, 2024

Good job! Curious whether it is possible to modify

to make it work on windows? e.g. https://superuser.com/questions/624368/how-to-chmod-x-a-file-in-windows-for-use-in-linux which discusses scenario when using git

@canxin121
Copy link
Contributor Author

Good job! Curious whether it is possible to modify 好工作!好奇是否可以修改

to make it work on windows? e.g. https://superuser.com/questions/624368/how-to-chmod-x-a-file-in-windows-for-use-in-linux which discusses scenario when using git
让它在Windows上工作?例如https://superuser.com/questions/624368/how-to-chmod-x-a-file-in-windows-for-use-in-linux 讨论使用 git 时的场景

This shouldn't be possible as far as I know, the file permission systems of win and unix are very different, and even if you use Git Bash on win to chmod a+x some.sh, you'll find that git doesn't recognize that some.sh has changed.

Yet the sh permissions information can actually be uploaded to Github, which is why if you flutter_rust_bridge_codegen.exe create new_app on macOS and then go on to continue the process of reproducing the problem as mentioned in the issue, you'll find that the problem disappears.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 28, 2024

I am considering the

git update-index --chmod=+x myfile.sh

in the https://superuser.com/questions/624368/how-to-chmod-x-a-file-in-windows-for-use-in-linux. Have you tried it on windows? (I am on mac so does not have a convenient environment to have a try)

@canxin121
Copy link
Contributor Author

canxin121 commented Mar 28, 2024

I am considering the

git update-index --chmod=+x myfile.sh

in the https://superuser.com/questions/624368/how-to-chmod-x-a-file-in-windows-for-use-in-linux. Have you tried it on windows? (I am on mac so does not have a convenient environment to have a try)

But if you want to use git to change the permissions on the sh file, this requires that you have git version 2.9.0 or higher on this computer in order to do so, and I can't say for sure that useing git to change the permissions on the sh file retains the permissions information correctly when you don't use git to move files.(e.g., when you copy the file directly from Win to Unix without the .git folder)

Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.76%. Comparing base (d938603) to head (3bab8a9).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1842      +/-   ##
==========================================
- Coverage   99.27%   97.76%   -1.51%     
==========================================
  Files         359      366       +7     
  Lines       15139    15283     +144     
==========================================
- Hits        15029    14942      -87     
- Misses        110      341     +231     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 28, 2024

I see. Then what about we use sh whatever_file.sh instead of whatever_file.sh, since the former does not require +x permission while the latter requires, like:

// old: "$BASEDIR/run_build_tool.sh" build-pod "$@"
sh "$BASEDIR/run_build_tool.sh" build-pod "$@"

To do so, I guess we can PR for upstream (cargokit), and then modify this PR to bump the cargokit version.

(I am a bit worried about the chmod-when-build solution, since that will change the files during a build when usually builds do not change things, and adding one extra stage looks a bit heavy)

@canxin121
Copy link
Contributor Author

I see. Then what about we use sh whatever_file.sh instead of whatever_file.sh, since the former does not require +x permission while the latter requires, like:

// old: "$BASEDIR/run_build_tool.sh" build-pod "$@"
sh "$BASEDIR/run_build_tool.sh" build-pod "$@"

To do so, I guess we can PR for upstream (cargokit), and then modify this PR to bump the cargokit version.

(I am a bit worried about the chmod-when-build solution, since that will change the files during a build when usually builds do not change things, and adding one extra stage looks a bit heavy)

It makes sense to do this if you want to change the upstream library, I added a step to change the permissions just because I didn't want to additionally change the upstream cargokit.
I tend to change a much smaller range of code. Also I can't figure out if there's any problem left over from the chmod-when-build solution , since I think both podspec and sh file are things that codegen out at the same time and don't need to be changed for users who don't care about the build process.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 28, 2024

It makes sense to do this if you want to change the upstream library, I added a step to change the permissions just because I didn't want to additionally change the upstream cargokit.

No worries, it is common to change upstream dependencies, because by doing so, whoever using that library but not using flutter_rust_bridge will also enjoy the new feature/fix - which is good for the whole community.

I tend to change a much smaller range of code. Also I can't figure out if there's any problem left over from the chmod-when-build solution , since I think both podspec and sh file are things that codegen out at the same time and don't need to be changed for users who don't care about the build process.

Totally agree, thus adding sh to upstream seems most minimal and avoids the possibility of chmod problem.

@canxin121
Copy link
Contributor Author

It makes sense to do this if you want to change the upstream library, I added a step to change the permissions just because I didn't want to additionally change the upstream cargokit.

No worries, it is common to change upstream dependencies, because by doing so, whoever using that library but not using flutter_rust_bridge will also enjoy the new feature/fix - which is good for the whole community.

I tend to change a much smaller range of code. Also I can't figure out if there's any problem left over from the chmod-when-build solution , since I think both podspec and sh file are things that codegen out at the same time and don't need to be changed for users who don't care about the build process.

Totally agree, thus adding sh to upstream seems most minimal and avoids the possibility of chmod problem.

Ok, I'll leave the upstream changes to you, since I'm really not familiar with the sh code and I don't have a mac to test it on.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Mar 28, 2024

If you insist I will do that - but it is you who made the initial contribution, so I do not want to take your credit!

@canxin121
Copy link
Contributor Author

If you insist I will do that - but it is you who made the initial contribution, so I do not want to take your credit!

I'll try that then, I think I can use github action to test if it's working properly.

@canxin121
Copy link
Contributor Author

canxin121 commented Mar 28, 2024

Upstream Pr Tracking: irondash/cargokit#65

A temporary normal working branch by switching the cargokit submodule to my cargokit branch: https://github.com/canxin121/flutter_rust_bridge/tree/test

fzyzcjy and others added 11 commits April 2, 2024 10:15
Fix class is not generated when having only static methods; Fix passing non-existent variable to getter causing compilation error; Fix missing code generation when using enum and methods
knopp pushed a commit to irondash/cargokit that referenced this pull request Apr 3, 2024
Origin issue: fzyzcjy/flutter_rust_bridge#1840
Origin pr: fzyzcjy/flutter_rust_bridge#1842

Simple problem description: if you create a new flutter_rust project in
windows, the sh script is automatically created, but the executable
permissions cannot be set. When moving this project to macOS and
executing `flutter build ios` to build ipa files the process crashes due
to insufficient sh script permissions.

This permission problem can be circumvented by `sh script.sh` rather
than `script.sh`

As verified by Github Action's macOS Runner, the modified cargokit works
fine for building ipa apps using the flutter_rust project created on
windows.

[Action
Link](https://github.com/canxin121/new_app_test/actions/runs/8466294309)
@canxin121
Copy link
Contributor Author

The upstream pr has been merged, and I updated the commit of the cargokit submodule.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Apr 3, 2024

LGTM! Maybe try

./frb_internal generate-run-frb-codegen-command-integrate --package frb_example/flutter_via_create
./frb_internal generate-run-frb-codegen-command-integrate --package frb_example/flutter_via_integrate

to update the corresponding files

@canxin121
Copy link
Contributor Author

canxin121 commented Apr 3, 2024

LGTM! Maybe try LGTM!

./frb_internal generate-run-frb-codegen-command-integrate --package frb_example/flutter_via_create
./frb_internal generate-run-frb-codegen-command-integrate --package frb_example/flutter_via_integrate

to update the corresponding files

It failed to run.

PS D:\Git\flutter_rust_bridge> ./frb_internal generate-run-frb-codegen-command-integrate --package frb_example/flutter_via_create
Building package executable... 
Built flutter_rust_bridge_internal:flutter_rust_bridge_internal.
Pick temporary directory: D:\Git\flutter_rust_bridge\target\GenerateRunFrbCodegenCommandIntegrate\temp_2024-04-03T231148317263_744882832
Unhandled exception:
PathAccessException: Rename failed, path = 'D:\Git\flutter_rust_bridge\frb_example/flutter_via_create' (OS Error: 拒绝访问。
, errno = 5) (The meaning of the preceding sentence in Chinese is '(OS Error: Access Denied. , errno = 5)')
#0      _checkForErrorResponse (dart:io/common.dart:55:9)
#1      _Directory.rename.<anonymous closure> (dart:io/directory_impl.dart:195:7)
<asynchronous suspension>
#2      generateRunFrbCodegenCommandIntegrate.<anonymous closure> (package:flutter_rust_bridge_internal/src/makefile_dart/generate.dart:334:7)
<asynchronous suspension>
#3      wrapMaybeSetExitIfChangedRaw (package:flutter_rust_bridge_internal/src/makefile_dart/generate.dart:411:3)
<asynchronous suspension>
#4      _wrapMaybeSetExitIfChanged (package:flutter_rust_bridge_internal/src/makefile_dart/generate.dart:400:3)
<asynchronous suspension>
#5      generateRunFrbCodegenCommandIntegrate (package:flutter_rust_bridge_internal/src/makefile_dart/generate.dart:317:3)
<asynchronous suspension>
#6      SimpleConfigCommand.run (package:flutter_rust_bridge_internal/src/utils/makefile_dart_infra.dart:88:31)
<asynchronous suspension>
#7      CommandRunner.runCommand (package:args/command_runner.dart:212:13)
<asynchronous suspension>
#8      main (file:///D:/Git/flutter_rust_bridge/tools/frb_internal/bin/flutter_rust_bridge_internal.dart:30:3)
<asynchronous suspension>

I tried using attrib and icacls to change the file permissions to all users having control permissions, but it didn't work.

@fzyzcjy
Copy link
Owner

fzyzcjy commented Apr 3, 2024

@canxin121
Copy link
Contributor Author

canxin121 commented Apr 4, 2024

That's weird... Another way, if the local env somehow fails like that, is to copy-paste the diff e.g. https://github.com/fzyzcjy/flutter_rust_bridge/actions/runs/8539956598/job/23395926863?pr=1842 and apply it https://stackoverflow.com/questions/12320863/how-do-you-take-a-git-diff-file-and-apply-it-to-a-local-branch-that-is-a-copy-o.

I successfully executed the command with github action and downloaded all the files, then realized that none of the files had been modified, except for the difference between LF and CRLF

@fzyzcjy
Copy link
Owner

fzyzcjy commented Apr 4, 2024

Well, I mean just save this to e.g. a.patch:

diff --git a/frb_example/flutter_via_create/rust_builder/cargokit/build_pod.sh b/frb_example/flutter_via_create/rust_builder/cargokit/build_pod.sh
index 379b9c6..ed0e0d9 100755
--- a/frb_example/flutter_via_create/rust_builder/cargokit/build_pod.sh
+++ b/frb_example/flutter_via_create/rust_builder/cargokit/build_pod.sh
@@ -49,7 +49,7 @@ do
   fi
 done
 
-"$BASEDIR/run_build_tool.sh" build-pod "$@"
+sh "$BASEDIR/run_build_tool.sh" build-pod "$@"
 
 # Make a symlink from built framework to phony file, which will be used as input to
 # build script. This should force rebuild (podspec currently doesn't support alwaysOutOfDate
diff --git a/frb_example/flutter_via_create/rust_builder/cargokit/build_tool/lib/src/android_environment.dart b/frb_example/flutter_via_create/rust_builder/cargokit/build_tool/lib/src/android_environment.dart
index 2b0b712..15fc9ee 100644
--- a/frb_example/flutter_via_create/rust_builder/cargokit/build_tool/lib/src/android_environment.dart
+++ b/frb_example/flutter_via_create/rust_builder/cargokit/build_tool/lib/src/android_environment.dart
@@ -118,7 +118,7 @@ class AndroidEnvironment {
 
     final cxxKey = 'CXX_${target.rust}';
     final cxxValue = path.join(toolchainPath, 'clang++$exe');
-    final cxxfFlagsKey = 'CXXFLAGS_${target.rust}';
+    final cxxFlagsKey = 'CXXFLAGS_${target.rust}';
     final cxxFlagsValue = targetArg;
 
     final linkerKey =
@@ -155,7 +155,7 @@ class AndroidEnvironment {
       ccKey: ccValue,
       cfFlagsKey: cFlagsValue,
       cxxKey: cxxValue,
-      cxxfFlagsKey: cxxFlagsValue,
+      cxxFlagsKey: cxxFlagsValue,
       ranlibKey: ranlibValue,
       rustFlagsKey: rustFlagsValue,
       linkerKey: selfPath,
diff --git a/frb_example/flutter_via_create/rust_builder/cargokit/cmake/cargokit.cmake b/frb_example/flutter_via_create/rust_builder/cargokit/cmake/cargokit.cmake
index 8832600..ddd05df 100644
--- a/frb_example/flutter_via_create/rust_builder/cargokit/cmake/cargokit.cmake
+++ b/frb_example/flutter_via_create/rust_builder/cargokit/cmake/cargokit.cmake
@@ -50,6 +50,7 @@ function(apply_cargokit target manifest_dir lib_name any_symbol_name)
     else()
         set(SCRIPT_EXTENSION ".sh")
         set(IMPORT_LIB_EXTENSION "")
+        execute_process(COMMAND chmod +x "${cargokit_cmake_root}/run_build_tool${SCRIPT_EXTENSION}")
     endif()
 
     # Using generators in custom command is only supported in CMake 3.20+
@@ -75,6 +76,7 @@ function(apply_cargokit target manifest_dir lib_name any_symbol_name)
         )
     endif()
 
+
     set_source_files_properties("${CMAKE_CURRENT_BINARY_DIR}/_phony_" PROPERTIES SYMBOLIC TRUE)
 
     if (TARGET ${target})
@@ -94,4 +96,4 @@ function(apply_cargokit target manifest_dir lib_name any_symbol_name)
     # Allow adding the output library to plugin bundled libraries
     set("${target}_cargokit_lib" ${OUTPUT_LIB} PARENT_SCOPE)
 
-endfunction()
\ No newline at end of file
+endfunction()
diff --git a/frb_example/flutter_via_create/rust_builder/cargokit/gradle/plugin.gradle b/frb_example/flutter_via_create/rust_builder/cargokit/gradle/plugin.gradle
index 28636e9..2fc1d70 100644
--- a/frb_example/flutter_via_create/rust_builder/cargokit/gradle/plugin.gradle
+++ b/frb_example/flutter_via_create/rust_builder/cargokit/gradle/plugin.gradle
@@ -58,7 +58,13 @@ abstract class CargoKitBuildTask extends DefaultTask {
         def manifestDir = Paths.get(project.buildscript.sourceFile.parent, project.cargokit.manifestDir)
 
         def rootProjectDir = project.rootProject.projectDir
-
+        
+        if (!Os.isFamily(Os.FAMILY_WINDOWS)) {
+            project.exec {
+                commandLine 'chmod', '+x', path
+            }
+        }
+        
         project.exec {
             executable path
             args "build-gradle"
diff --git a/frb_example/flutter_via_create/rust_builder/cargokit/run_build_tool.sh b/frb_example/flutter_via_create/rust_builder/cargokit/run_build_tool.sh
index 0e2c973..6e[594](https://github.com/fzyzcjy/flutter_rust_bridge/actions/runs/8539956598/job/23395926863?pr=1842#step:7:595)a2 100755
--- a/frb_example/flutter_via_create/rust_builder/cargokit/run_build_tool.sh
+++ b/frb_example/flutter_via_create/rust_builder/cargokit/run_build_tool.sh
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/usr/bin/env bash
 
 set -e
 
@@ -42,6 +42,12 @@ void main(List<String> args) {
 }
 EOF
 
+# Create alias for `shasum` if it does not exist and `sha1sum` exists
+if ! [ -x "$(command -v shasum)" ] && [ -x "$(command -v sha1sum)" ]; then
+  shopt -s expand_aliases
+  alias shasum="sha1sum"
+fi
+
 # Dart run will not cache any package that has a path dependency, which
 # is the case for our build_tool_runner. So instead we precompile the package
 # ourselves.
diff --git a/frb_example/flutter_via_create/rust_builder/ios/rust_lib_flutter_via_create.podspec b/frb_example/flutter_via_create/rust_builder/ios/rust_lib_flutter_via_create.podspec
index 30f5eb2..8a78[601](https://github.com/fzyzcjy/flutter_rust_bridge/actions/runs/8539956598/job/23395926863?pr=1842#step:7:602) 100644
--- a/frb_example/flutter_via_create/rust_builder/ios/rust_lib_flutter_via_create.podspec
+++ b/frb_example/flutter_via_create/rust_builder/ios/rust_lib_flutter_via_create.podspec
@@ -42,4 +42,4 @@ A new Flutter FFI plugin project.
     'EXCLUDED_ARCHS[sdk=iphonesimulator*]' => 'i386',
     'OTHER_LDFLAGS' => '-force_load ${BUILT_PRODUCTS_DIR}/librust_lib_flutter_via_create.a',
   }
-end
+end
\ No newline at end of file
diff --git a/frb_example/flutter_via_create/rust_builder/macos/rust_lib_flutter_via_create.podspec b/frb_example/flutter_via_create/rust_builder/macos/rust_lib_flutter_via_create.podspec
index d2bbc3b..9403cf8 100644
--- a/frb_example/flutter_via_create/rust_builder/macos/rust_lib_flutter_via_create.podspec
+++ b/frb_example/flutter_via_create/rust_builder/macos/rust_lib_flutter_via_create.podspec
@@ -41,4 +41,4 @@ A new Flutter FFI plugin project.
     'EXCLUDED_ARCHS[sdk=iphonesimulator*]' => 'i386',
     'OTHER_LDFLAGS' => '-force_load ${BUILT_PRODUCTS_DIR}/librust_lib_flutter_via_create.a',
   }
-end
+end

and git apply that patch.

Anyway, I may just run it later when merging this PR.

@fzyzcjy fzyzcjy changed the title fix sh permission. Fix sh permission Apr 4, 2024
@fzyzcjy fzyzcjy changed the base branch from master to feat/1842 April 4, 2024 09:13
@fzyzcjy fzyzcjy merged commit c2fc2f8 into fzyzcjy:feat/1842 Apr 4, 2024
91 of 98 checks passed
Copy link

welcome bot commented Apr 4, 2024

Hi! Congrats on merging your first pull request! 🎉

@fzyzcjy
Copy link
Owner

fzyzcjy commented Apr 4, 2024

@all-contributors please add @canxin121 for code

Copy link
Contributor

@fzyzcjy

I've put up a pull request to add @canxin121! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problems with .sh script permissions
2 participants