Skip to content
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

feat: use ShellExecuteW for detached spawning on Windows #91

Merged

Conversation

amrbashir
Copy link
Contributor

@amrbashir amrbashir commented Feb 29, 2024

@Legend-Master
Copy link

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

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

I think this is great work, thanks so much for contributing!
And all that is done without adding a new crate, just by declaring external linkage of a C function. I wonder how it knows what to link to, it seems to be implied.

In any case, I will be merging this under the assumption that it was validated on Windows, and create a new release promptly: https://github.com/Byron/open-rs/releases/tag/v5.1.0

@Byron Byron merged commit b268647 into Byron:main Mar 1, 2024
5 checks passed
@Byron
Copy link
Owner

Byron commented Mar 2, 2024

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
Copy link
Owner

Byron commented Mar 2, 2024

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
Copy link

Maybe because of incremental build? I can build and link it on current head 21a73ee 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
Copy link
Contributor Author

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
Copy link
Owner

Byron commented Mar 3, 2024

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 (74fd8ec). 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
Copy link
Contributor Author

amrbashir commented Mar 4, 2024

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

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.

Can't open file if the file is being used by a program on Windows
3 participants