Skip to content

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

Merged
merged 6 commits into from
Feb 5, 2025
Merged

Conversation

wx257osn2
Copy link
Contributor

related: #1

@wx257osn2 wx257osn2 changed the title Support Windows WIP: Support Windows Jan 23, 2025
@cordx56
Copy link
Owner

cordx56 commented Feb 4, 2025

This resolved in the commit https://github.com/cordx56/rustowl/tree/b190ebfc8289e025f62a898cb0d0a840238c0776 .
I will close this PR if someone confirmed working in Windows.
Related: #1

@wx257osn2 wx257osn2 changed the title WIP: Support Windows Support Windows Feb 4, 2025
@wx257osn2 wx257osn2 changed the title Support Windows cargo owl Supports Windows Feb 4, 2025
Copy link
Owner

@cordx56 cordx56 left a 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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toolchain_version をコミット忘れしていませんか?

Copy link
Contributor Author

@wx257osn2 wx257osn2 Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

アホ 😇 後で追加します 🙇

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

追加しました

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちなみに理想は rust-toolchain.tomlchannel から自動生成することだと思っていて(管理対象が減るので)、ただ差分がなかったときには変更せず、みたいなのを真面目に build.rs で作り込むのも大げさだよなとなり手を付けてませんのメモ書きを残しておきます(じゃすとあいであというやつなのでやってもやらなくてもよいです)

@@ -12,28 +16,26 @@ fn main() {
#[cfg(windows)]
unsafe {
Copy link
Owner

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 外すのをお願いしたいです。

Copy link
Contributor Author

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ではないですか?(たぶんスレッドセーフでないため)(なんか勘違いしてたら教えてください)

Copy link
Owner

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

とりあえずここはそのままで大丈夫です。すみません🙇

Copy link
Contributor Author

@wx257osn2 wx257osn2 Feb 5, 2025

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 を使う)方法に変更

@cordx56
Copy link
Owner

cordx56 commented Feb 5, 2025

I confirmed that this works with Windows and macOS. Thank you for your contribution, @wx257osn2 !

@cordx56 cordx56 merged commit 28c87bd into cordx56:main Feb 5, 2025
@wx257osn2 wx257osn2 deleted the support-windows branch February 5, 2025 08:07
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.

2 participants