Skip to content

Fix GNU toolchain builds #166

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 2 commits into from
May 27, 2020
Merged

Fix GNU toolchain builds #166

merged 2 commits into from
May 27, 2020

Conversation

Alovchin91
Copy link
Contributor

@Alovchin91 Alovchin91 commented May 27, 2020

This PR fixes the GNU toolchain (-pc-windows-gnu) builds for the apps that use winrt-rs.

Note that it is still not possible to build all the tests in winrt-rs itself with the GNU toolchain, but with MSVC toolchain all the tests pass.

It is probably a temporary workaround, though I see that the TODO to dynamically load the APIs from OneCore has been removed.

@msftclas
Copy link

msftclas commented May 27, 2020

CLA assistant check
All CLA requirements met.

@@ -22,7 +22,7 @@ extern "system" {
) -> u32;
}

#[link(name = "onecore")]
#[link(name = "windowsapp")]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know why the GNU toolchain accepts windowsapp and not onecore? Both are just umbrella libs. And yes, I'm planning to avoid the problem with these libs with #142. This change is ok, but in general I'm unconfortable making a change that we don't verify in the build. Is there a way to validate both MSVC and GNU?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is because there is libwindowsapp.a in the MinGW-w64 toolchain but no libonecore.a - as simple as that :)

I can play with the tests to see if I can compile them for GNU, but the reason is quite similar - there is no libcoremessaging.a in the GNU toolchain.

Copy link
Contributor Author

@Alovchin91 Alovchin91 May 27, 2020

Choose a reason for hiding this comment

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

The approach in #142 is definitely much better.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can probably simplify the tests to avoid the additional link dependencies. The concern I have with #142 is that I don't know whether that will work with something other than the MSVC lib tool.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only usage of coremessaging is for CreateDispatcherQueueController. This is in composition.rs tests. I'm not sure how this can be simplified tbh, but we could probably load this function dynamically instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The approach in #142 should work for MinGW-w64 just fine from my understanding. What you need is a *.def file and the dlltool that will generate a *.a file based on it. To automate *.def file generation you could also use gendef first. Example (from here):

gendef.exe csfml-graphics-2.dll
dlltool.exe -d csfml-graphics-2.def -D csfml-graphics-2.dll -l libcsfml-graphics.a

These tools are part of MinGW-w64 toolchain and should be available on all platforms.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's take this change for now and deal with the tests separately. It would be nice to have a simple LoadLibrary/GetProcAddress helper.

@kennykerr kennykerr merged commit 099e458 into microsoft:master May 27, 2020
@Alovchin91 Alovchin91 deleted the alovchin91/fix-gnu-toolchain-build branch May 27, 2020 17:44
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.

3 participants