WINAPIV

That would require adding more dependencies on Windows.h than needed.
I believe we should not add this complexity.

True. That's why things are like they are right now, but we have ended up facing the same issue by keeping __stdcall as the absolute default. I'll ping you offline to elaborate, for the concerns are on dependent projects and not directly on RNW.

@JunielKatarn is this in reference to the OpenSSL break?

Correct.
Also, OpenSSL is a sensitive dependency, so it's preferable to not patch it, even if other components need to be patched instead.

I'm aware of dependent software being potentially impacted.
I'll start testing these changes now.

WINAPIV is declared inside of MinWinDef.h, not Windows.h

From quickly taking a look of contents of MinWinDef.h, it looks fairly small and self contained

In reply to: 587938026 [](ancestors = 587938026)

I believe we would also need to fork (and maintina) more React Native APIs in ReactCommon that we export, but this change doesn't seem to do that. E.g. the instance management stuff we call directly in devmain.

I will be testing this today.
Note, if these changes affect depending projects, we can partially revert this change by:

• Going back to ReactWindows.OpenSSL.StdCall.Static.
• Setting CallingConvention back to StdCall.

I'll merge this change after addressing feedback.

@JunielKatarn apart from changing defs, do we need to change OpenSSL source at all, or just change compiler flags when producing the x86 binary?

I mentioned needing to patch more in ReactCommon, but as soon as we add all React, Yoga, and Folly sources exporting anything as patches, we're going back to near square one for Native deforking efforts, and people will almost definitely patch our code even note. I was just about to remove some of the last of the patches there.

Given we were going to remove the rest, would you be willing to take efforts to merge during upgrades, or build tooling we'll need to detect when these are out of date when we're pulling from master? Without exaggeration, this change has the potential to add engineer-months of extra effort in the future relative to changing only OpenSSL.

Either way, I agree with @kmelmon that this change has large enough implications that we should discuss with everyone involved. Checkin should be blocked until this happens.

@JunielKatarn apart from changing defs, do we need to change OpenSSL source at all, or just change compiler flags when producing the x86 binary?

@NickGerleman With this change, OpenSSL can be built "off the shelf" with not a single change to sources or compiler flags.
Package ReactWindows.OpenSSL.v141.Static already reflects this.

I talked to @JunielKatarn offline. We shouldn't need to patch React Native functions, who are using "thiscall". Since there's not a sane way around this for OpenSSL this can make sense as a stopgap. @JunielKatarn will take on ownership of maintaining the patches during upgrades, and upstreaming them.

Hello @JunielKatarn!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Folly isn't designed to be a DLL: it doesn't have an ABI, or versions.

The current groupthink internally is that if some project wants to make a folly DLL for their own reasons then it's OK for them to have the burden of choosing the calling convention with the appropriate /G flag and making sure they are using the same one consistently in their project.

Agreed. We should just wait for the work to remove folly from the API boundary before we switch the calling convention used inside react-native-windows back to normal.

Folly isn't designed to be a DLL: it doesn't have an ABI, or versions.

The current groupthink internally is that if some project wants to make a folly DLL for their own reasons then it's OK for them to have the burden of choosing the calling convention with the appropriate /G flag and making sure they are using the same one consistently in their project.

Problem is, we found a case where there are two projects (Say A which depends on B, and both depend on Folly, which is statically linked into B) that use different calling conventions.
Thus, whenever A calls into a Folly method, it will try to load it with a different calling convention, and we'll hit runtime crashes.

Annotating the proper calling convention would ensure whichever /G switch is used won't affect any consuming projects.
That said, if my team judges this as non-urgent, we can drop the subject for now.

we found a case where there are two projects (Say A which depends on B, and both depend on Folly, which is statically linked into B) that use different calling conventions.

The model we expect here is for both A and B to statically link to folly and that neither of them should be exporting any folly symbols.

we found a case where there are two projects (Say A which depends on B, and both depend on Folly, which is statically linked into B) that use different calling conventions.

The model we expect here is for both A and B to statically link to folly and that neither of them should be exporting any folly symbols.

We'll look into that :)