-
-
Notifications
You must be signed in to change notification settings - Fork 103
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
@@ -12,28 +16,26 @@ fn main() { | |||
#[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