-
Notifications
You must be signed in to change notification settings - Fork 301
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
Fix sh permission #1842
Conversation
Hi! Thanks for opening this pull request! 😄 |
As verified by MacOS Runner using Github Actions, this pr works fine and resolves the previous permissions issue. |
Good job! Curious whether it is possible to modify
|
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 Yet the sh permissions information can actually be uploaded to Github, which is why if you |
I am considering the
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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
I see. Then what about we use
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. |
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.
Totally agree, thus adding |
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. |
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. |
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 |
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
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)
The upstream pr has been merged, and I updated the commit of the cargokit submodule. |
LGTM! Maybe try
to update the corresponding files |
It failed to run.
I tried using |
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 |
Well, I mean just save this to e.g.
and git apply that patch. Anyway, I may just run it later when merging this PR. |
Hi! Congrats on merging your first pull request! 🎉 |
@all-contributors please add @canxin121 for code |
I've put up a pull request to add @canxin121! 🎉 |
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
./frb_internal precommit --mode slow
(orfast
) is run (it internal runs code generator, does auto formatting, etc)../website
folder) are updated.