Uh oh!
There was an error while loading. Please reload this page.
[mono] Reorganize zlib dependency logic - #63365
Merged
Merged
Conversation
In particular this re-enables embedded PDB support on iOS and Android
lambdageek
requested review from
BrzVlad, SamMonoRT, imhameed, marek-safar and vargaz
as code ownersJanuary 4, 2022 21:44
lambdageek
commented
Jan 4, 2022
Uh oh!
There was an error while loading. Please reload this page.
lambdageek
commented
Jan 4, 2022
MemberAuthor
/azp run runtime-manual |
|
Azure Pipelines successfully started running 1 pipeline(s). |
thaystg
previously approved these changes
Jan 5, 2022
lambdageek
marked this pull request as draft
January 5, 2022 14:21
lambdageek
commented
Jan 5, 2022
MemberAuthor
See #63365 (comment) I think we can do better for net7: have explicit options for enable/disable Embedded PDB support and compressed log profiler output, and use those to decide whether zlib is required or not, and if it is required, whether we should depend on the system zlib, or the in-tree copy. |
Instead of deciding whether features are enabled based on the presence or absense of zlib, add switches to control features (embedded PDB support, and compressed log profiler output) and based on that require zlib. On some platforms we use the in-tree copy of zlib from src/native/external/zlib, otherwise we want to link against the system zlib. Previously, if zlib was missing the build would quietly succeed, but the feature support would be missing. Now, if the feature isn't disabled but zlib is missing, we fail the build. If both features are disabled, don't depend on zlib at all.
lambdageekforce-pushed
the
have-sys-zlib-main
branch
from
January 5, 2022 17:21
bfb11b7 to
2f7c714Comparelambdageek
marked this pull request as ready for review
January 5, 2022 17:31
am11
reviewed
Jan 5, 2022
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| #cmakedefine DISABLE_QCALLS 1 | ||
| /* Embedded PDB support disabled */ | ||
| #cmakedefine DISABLE_EMBEDDED_PDB 1 |
Member
There was a problem hiding this comment.
nit: when DISABLE_EMBEDDED_PDB is defined in cmake config, this produces:
#defineDISABLE_EMBEDDED_PDB 1in coreclr, src/native etc., we use these two variants:
#cmakedefine TOKEN
#cmakedefine01 TOKENwhen TOKEN is defined in cmake config, it generates:
#defineTOKEN// usage: #ifdef TOKEN#defineTOKEN 1 // usage: #if TOKENand when undefined, produces:
// nothing for the first line#defineTOKEN 0 // usage: #if TOKEN- reverse if() logic to avoid negation - use cmakedefine without a value in config.h.in, same as CoreCLR
steveisok
self-requested a review
January 6, 2022 21:14
steveisok
approved these changes
Jan 6, 2022
steveisok pushed a commit
that referenced
this pull request
Jan 10, 2022
In particular this re-enables embedded PDB support on iOS and Android The corresponding PR for main is #63365
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Instead of deciding whether features are enabled based on the presence or absence of zlib, add switches to control features (embedded PDB support, and compressed log profiler output) and based on that require zlib.
On some platforms we use the in-tree copy of zlib from src/native/external/zlib, otherwise we want to link against the system zlib.
Previously, if zlib was missing the build would quietly succeed, but the feature support would be missing. Now, if the feature isn't disabled but zlib is missing, we fail the build.
If both features are disabled, don't depend on zlib at all.
In particular this re-enables embedded PDB support on iOS and Android
The corresponding release/6.0-maui PR is #63359 and is more conservative - it just duplicates the mono/mono HAVE_SYS_ZLIB check. In this PR, we get rid of
HAVE_SYS_ZLIBentirely.