Skip to content

Fix x86_64-pc-windows-gnu target build#37

Closed
lmkra wants to merge 7 commits intomuja:masterfrom
lmkra:fix-windows-gnu-build
Closed

Fix x86_64-pc-windows-gnu target build#37
lmkra wants to merge 7 commits intomuja:masterfrom
lmkra:fix-windows-gnu-build

Conversation

@lmkra
Copy link
Contributor

@lmkra lmkra commented Oct 3, 2023

Latest version of unrar.rs fails to link on x86_64-pc-windows-gnu target.
Root cause is indirect dependency from underlying C++ library to MSVC-provided comsupp.lib.

Proposed patch works around it by enabling this dependency only when using MSVC compiler.
It does drop some Windows version detection heuristics on -gnu targets, but I haven't observed any issues with it during tests.

Additionally, even after linking is fixed using above method, produced build fails during runtime with memory access violation error. This is most probably caused by type size mismatch (32 vs 64bits).
Enabling winapi crate usage for all flavours of windows target fixes the issue.

@lmkra lmkra marked this pull request as ready for review October 5, 2023 21:46
@muja
Copy link
Owner

muja commented Oct 7, 2023

Hello
I had noticed MSVC seemed to not build, but I didn't know how to fix it. So thank you for this PR!

You've touched the CPP files (e.g. isnt.cpp). Did you update the RAR version or did you edit them manually?

Also the build is failing for OS X 10.8, I've specifically added that to the build matrix because of one user of the lib. I'll investigate that.

Finally, since you have no contributions yet. I'm in the process of changing the license of this library. do you agree with (re-)licensing all your contributions to this repository under the licenses MIT and Apache2, at the users' option?

@lmkra
Copy link
Contributor Author

lmkra commented Oct 7, 2023

Hi,

I've edited CPP files manually - as far as I understand, their licence permits it.
There should be no functional difference compared to previous version, with one exception of non-MSVC windows build (e.g. MinGW), which after my changes will have simpler Windows 10 version detection mechanism (in order not to depend on MSVC libs).

Failure on OS X 10.8 is a bit unexpected, but fortunately should be easily fixable - I'll push the change shortly.

I agree with licensing all my contributions to this repository under MIT and Apache 2 licences, at the users' option.

@muja
Copy link
Owner

muja commented Oct 7, 2023

Yes the license permits it. However to avoid regressions once we update to a new version, we have to reapply our changes so they’re not overwritten. That’s why I asked. I assume there is no other way to achieve this support?

@lmkra
Copy link
Contributor Author

lmkra commented Oct 8, 2023

At this moment I think this is the simplest solution to achieve windows-gnu support, but not the only one.

One potential alternative would be to implement missing comsupp symbols definitions in unrar_sys module,
but as it would mean to maintain a significant amount of C++ (or alternatively: unsafe rust FFI) code in unrar.rs, I considered it as not practically feasible.

I can limit the changes in unrar C++ sources just to isnt.cpp file to ease the maintenance burden when updating the library, but don't currently see a way to avoid it completely.

Best case scenario is that RarLab reworks Win11 detection mechanism in future versions of their library (currently this code is marked with Replace it with documented Windows 11 check when available. comment), then no patching will be necessary.

@muja
Copy link
Owner

muja commented Oct 8, 2023

Thank you. I'll merge it tomorrow. I'll edit/squash some of your commits if that's okay for you.

@lmkra
Copy link
Contributor Author

lmkra commented Oct 8, 2023

Sure thing.
Thank you very much :)

@lmkra
Copy link
Contributor Author

lmkra commented Nov 6, 2023

Hi,

I see both my PRs are still open. Is there something I can do to help prepare them for merge?

@muja
Copy link
Owner

muja commented Nov 8, 2023

I merged it manually through the command-line instructions. Unfortunately it didn't register it here, but that's no problem. I will release a new version soon.

@muja
Copy link
Owner

muja commented Nov 11, 2023

New version is on crates.io :) Thanks for contributing!

@lmkra lmkra deleted the fix-windows-gnu-build branch November 11, 2023 23:04
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