Byron / open-rs

Open a path or URL with the system-defined program
http://byron.github.io/open-rs
MIT License
319 stars 49 forks source link

feat: use `ShellExecuteW` for detached spawning on Windows #91

Closed amrbashir closed 8 months ago

amrbashir commented 8 months ago

closes #90

ref: https://github.com/tauri-apps/plugins-workspace/issues/1003

Legend-Master commented 8 months ago

I reopened #90, would you mind adding "Fix #90" to the description to auto close it?

Byron commented 8 months ago

Please note that I had to yank that release as it doesn't seem to be able to link in that function that is declared as having external linkage.

https://github.com/Byron/gitoxide/actions/runs/8124066000/job/22205141131?pr=1236#step:7:446

I am using this PR to try to reproduce the problem, but yanking the version in question definitely helped to fix gitoxide's CI.

Byron commented 8 months ago

It looks like I am unable to reproduce the issue :/, maybe you can take a look? I thought it's about win32 targets, but doesn't seem to be the case. Maybe it's some interplay of gitoxide which definitely pulls in windows-sys itself.

Thanks for your help! I am quite aware that yanking v5.1 might break others who are relying on it, it's like being caught between a rock and a hard place.

Legend-Master commented 8 months ago

Maybe because of incremental build? I can build and link it on current head 21a73ee1 with windows-sys

windows-sys = { version = "0.52", features = [
    "Win32_Foundation",
    "Win32_Security_Authorization",
    "Win32_Storage_FileSystem",
    "Win32_System_Memory",
    "Win32_System_Threading",
] }

image

amrbashir commented 8 months ago

Please note that I had to yank that release as it doesn't seem to be able to link in that function that is declared as having external linkage.

Byron/gitoxide/actions/runs/8124066000/job/22205141131?pr=1236#step:7:446

I am using this PR to try to reproduce the problem, but yanking the version in question definitely helped to fix gitoxide's CI.

The defined extern is almost identical to the one generated by windows-sys crate as you can see here. https://docs.rs/crate/windows-sys/latest/source/src/Windows/Win32/UI/Shell/mod.rs. I am not sure why it failed to link tbh but maybe a cache issue or a missing sdk? I can't test gitoxide my self but testing a project with open-rs from git and windows-sys and using the same function twice, once from manually linkage and once from windows-sys and both worked just fine.

Thanks for your help! I am quite aware that yanking v5.1 might break others who are relying on it, it's like being caught between a rock and a hard place.

You can publish 5.1.1 without this PR logic so users who depend on 5.1 won't have any issues.

Byron commented 8 months ago

The linking error, just for completeness, is this one:

  = note: libopen-63f38b321dddbc1d.rlib(open-63f38b321dddbc1d.open.aa6c606d350ade25-cgu.0.rcgu.o) : error LNK2019: unresolved external symbol __imp__ShellExecuteW referenced in function __ZN4open7windows13ShellExecuteW17hac275a6d18dced78E

          D:\a\gitoxide\gitoxide\install-artifacts\i686-pc-windows-msvc\debug\deps\gix-0c420fdae7e7e989.exe : fatal error LNK1120: 1 unresolved externals

I'd be inclined to say that somehow the symbol it tries to link to is prefixed with __imp__ which is maybe causing the error. I vaguely recall that the flate2 crate can rename symbols, but it should only be able to do this with its own, of course.

Also I tried to reproduce the issue locally on a x64 Windows VM, but to not avail.


In order to not leave open in a broken state, I put the new capability behind the shellexecute-on-windows feature toggle (74fd8ec005d9bd24e6cb604e3239730b0b414b84). It is now available on crates.io as version 5.1.1 as well, so @amrbashir can use it. Please let me know if anything doesn't work as expected.

amrbashir commented 8 months ago

Thanks, we will enable this feature on tauri-plugin-shell and see if we face any issues over time.