Handling of wincrypt.h conflicts in mingw-w64-android-tools

[meator]

Hey @Biswa96! Sorry if my pinging is inappropriate, but you are the only person whose commits have touched the offending patch file.

This discussion regards the following hunk of mingw-w64-android-tools/0002-android-tools-vendor.patch:

MINGW-packages/mingw-w64-android-tools/0002-android-tools-vendor.patch

Lines 1 to 16 in 6606ac9

	--- a/vendor/boringssl/include/openssl/target.h
	+++ b/vendor/boringssl/include/openssl/target.h
	@@ -78,8 +78,13 @@
	#define OPENSSL_APPLE
	#endif
	+// Unfortunate workaround to avoid symbol conflict with wincrypt.h
	+// See https://github.com/openssl/openssl/issues/9981
	#if defined(_WIN32)
	#define OPENSSL_WINDOWS
	+#undef PKCS7_SIGNER_INFO
	+#undef X509_EXTENSIONS
	+#undef X509_NAME
	#endif
	// Trusty and Android baremetal aren't Linux but currently define __linux__.

I've been testing the work in progress 37.0.0 update on my fork of nmeum/android-tools and I've noticed that since 36.0.1 there are more places where a X509_NAME collision occurs (at least one more TU fails to compile because of the name clashing in addition to the place already handled by the patch). This has brought the patch to my attention, since I (and you when 37.0.0 is released downstream) will have to update the patch.

I know it might be a bit early to comment on this, but you are a collaborator on the "upstream" downstream repository, so you are probably more aware of and actively involed in the topic discussed.

I propose the following: instead of adding the same fix (#undefing what has to be #undefed) to more places, I propose setting WIN32_LEAN_AND_MEAN or NOCRYPT where needed. I think that this is a more natural solution. boringssl itself uses WIN32_LEAN_AND_MEAN often, but it unfortunatelly doesn't export this macro (or NOCRYPT).

In my fork, I have determined that the libadb and adb targets (and no more) need either WIN32_LEAN_AND_MEAN or NOCRYPT to compile without the fix in the hunk. But my fork handles only a subset of the tools nmeum/android-tools handles (but I doubt that any of the miscelaneus utilities I do not build need boringssl¹).

To my limited knowledge, WIN32_LEAN_AND_MEAN only restricts what some standard headers like windows.h include, so its inclusion in libadb and adb shouldn't introduce any bugs. If any incompatibilities arose, they would lead to compilation errors (which is better than runtime errors). But there is still possibility that this could be a breaking change.

To my (again) limited knowledge, NOCRYPT works similarly to WIN32_LEAN_AND_MEAN, but it is much more targeted. I have verified that adding -DNOCRYPT to libadb and adb makes the subset of utilities my fork builds compile. This macro should be pretty safe and it completely resolves the issue.

If you agree, the change can be made immediatelly (i.e. onto mingw-w64-x86_64-android-tools-36.0.1), which will lead to a little less work during the 37.0.0 update. Or it can be addressed during the 37.0.0 update, leaving the current solution intact until then.

If you disagree and choose to #undef further during 37.0.0 update, I'd at least like to make a PR adding a patch comment header describing the situation to the patch. I already did some research on this, I'd like to at least improve documentation to justify it. Would such PR be considered even though it "does nothing"?

Footnotes

1. I have not verified this statement. ↩

[Biswa96]

Nice idea! I'll check that after 37.0.0.

[Biswa96]

I have created a PR to update android-tools to 37.0.0 and tried to add WIN32_LEAN_AND_MEAN the boringssl patch. It shows the same compiler error with X509_NAME.

[meator]

I just got my local version of my fork working on 37.0.0 and the changes I described (drop the linked hunk, add -DNOCRYPT to libadb and adb) worked.

Would there still be interest in this change? The package was updated already. It would make sense to push this change alongside an update, but this change in and of itself doesn't fix anything, it just alters the already working fix, so I don't think it makes as much sense to implement it now.

[Biswa96]

Could you share the change here just for reference? I am planning to add Windows support in android-tools project. If you have Windows device to test, it'd be very helpful.

[meator]

This was the first time I built a MSYS2 package, but the changes needed are fairly straight forward and reflect my proposals here (drop the hunk, add -DNOCRYPT to libadb and adb): 0001-android-tools-Use-NOCRYPT-instead-of-patch.patch

In the initial comment here, I claimed that another TU will be hit with X509_NAME clashes. As f80d666 proofs, that statement doesn't seem to be consistent with reality. I was testing changes on the then open 37.0.0 update PR in nmeum/android-tools. Things may have changed since then. Or the fairly minor changes unlikely to influence anything in f80d666 have fixed it.

Since the original patch is still adequate and fully solves the issue (since there are no more X509_NAME conflicts contrary to my predictions), changing things as I propose is even less justifiable. I recognize the wisdom of not fixing things which are not broken.

[Biswa96]

I think it's more preferable to add compiler flag in cmake than adding a random patch. So, I like your idea and will think about adding that.