-
-
Notifications
You must be signed in to change notification settings - Fork 108
cargo owl Supports Windows
#2
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
Conversation
|
This resolved in the commit https://github.com/cordx56/rustowl/tree/b190ebfc8289e025f62a898cb0d0a840238c0776 . |
b2298aa to
c866613
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
動くことは確認できました。ありがとうございます。
2か所だけコメントしてあるので、修正をお願いしたいです。
よろしくお願いします。
| @@ -1,7 +1,11 @@ | |||
| mod toolchain_version; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toolchain_version をコミット忘れしていませんか?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
アホ 😇 後で追加します 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
追加しました
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ちなみに理想は rust-toolchain.toml の channel から自動生成することだと思っていて(管理対象が減るので)、ただ差分がなかったときには変更せず、みたいなのを真面目に build.rs で作り込むのも大げさだよなとなり手を付けてませんのメモ書きを残しておきます(じゃすとあいであというやつなのでやってもやらなくてもよいです)
rustowl/src/main.rs
Outdated
| let target_dir = PathBuf::from(env::args().nth(3).unwrap_or("./target".to_owned())); | ||
|
|
||
| #[cfg(windows)] | ||
| unsafe { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここ、私がやった時になぜか env::set_var で動かなかったので unsafe + Win32 APIにしていたのですが、 env::set_var で動くことが確認できたので、 unsafe 外すのをお願いしたいです。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env::set_varくん、unsafe functionではないですか?(たぶんスレッドセーフでないため)(なんか勘違いしてたら教えてください)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あれ本当だ、なんで unsafe 外してコンパイル通ったんだろう?と思ったら、 “This function is also always safe to call on Windows” らしいので、もしかしたら cfg(windows) だとsafeで通るのか?となっています。
https://doc.rust-lang.org/std/env/fn.set_var.html
とりあえずここはそのままで大丈夫です。すみません🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set_var がunsafeになったの最近の話,みたいなのもちらっと見かけた ともかく set_var 以外はsafeなはずなので set_var 周辺以外はsafeにしておきました+ついでにより標準っぽい(split_paths / join_paths を使う)方法に変更
|
I confirmed that this works with Windows and macOS. Thank you for your contribution, @wx257osn2 ! |
related: #1