[mono-runtimes] Fix bundle*.zip use #510
Merged
Conversation
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
grendello
approved these changes
Mar 21, 2017
Contributor
Author
|
build |
2 similar comments
Contributor
|
build |
Contributor
Author
|
build |
radical
reviewed
Mar 22, 2017
| else # $(MSBUILD) != 1 | ||
| _CSC_EMITS_PDB := $(shell if pkg-config --atleast-version=4.9 mono ; then echo Pdb; fi ) | ||
| ifeq ($(_CSC_EMITS_PDB),Pdb) | ||
| MSBUILD_FLAGS += /p:_DebugFileExt=.pdb |
Member
There was a problem hiding this comment.
Are we setting this on the command line, in case we are building with xbuild? If yes, then we should do this only in that particular case.
Contributor
Author
There was a problem hiding this comment.
@radical: Yes, this is for the xbuild invocation case, when $(USE_MSBUILD) isn't 1.
jonpryor
force-pushed
the
jonp-use-bundle-76be1fb19c
branch
4 times, most recently
from
3e09b17 to
e962af7
Compare
Commit 76be1fb (6e9a661) broke `bundle*.zip` use. Consequently, `bundle*.zip` *is not used*, resulting in *yuuuge* build times (nearly 7 hours for full builds on macOS, over 2 hours for PRs!). The `_BuildUnlessCached` target calls the `GetMonoBundleItems` target, to determine whether the `ForceBuild` target -- which invokes `_BuildRuntimes` and many other targets -- should be executed. The `GetMonoBundleItems` returns all files which would be included in `bundle*.zip`. The cause of the break is that commit 6e9a661 changed the `GetMonoBundleItems` target to depend on the `_GetMonodocItems` target, which in turn depended on the `_BuildRuntimes` target. `_BuildRuntimes` is the target we want to avoid executing; it's the target that builds mono, and thus can take an eternity (particularly on the `xamarin-android` Jenkins lane, which builds **ALL THE ABIS**). Adding `_GetMonodocItems` in this fashion thus means we never use the cache, bceause in order to determine whether or not we can use the cache we need to do all the work that the cache was intended to avoid. Consider the [xamarin-android Build 312 log file][0] Building target "_BuildUnlessCached" in project ".../xamarin-android/build-tools/mono-runtimes/mono-runtimes.mdproj" (".../xamarin-android/build-tools/mono-runtimes/mono-runtimes.targets"); "Build" depends on it. ... Building target "GetMonoBundleItems" in project ".../xamarin-android/build-tools/mono-runtimes/mono-runtimes.mdproj" (".../xamarin-android/build-tools/mono-runtimes/mono-runtimes.targets"); "_BuildUnlessCached" depends on it. ... Building target "_GetMonodocItems" in project ".../xamarin-android/build-tools/mono-runtimes/mono-runtimes.mdproj" (".../xamarin-android/build-tools/mono-runtimes/mono-runtimes.targets"); "GetMonoBundleItems" depends on it. Building target "_BuildRuntimes" in project ".../xamarin-android/build-tools/mono-runtimes/mono-runtimes.mdproj" (".../xamarin-android/build-tools/mono-runtimes/mono-runtimes.targets"); "_GetMonodocItems" depends on it. ...and **BOOM**, the existence of `bundle*.zip` is meaningless. The fix is to partially revert commit 6e9a661, but the entire reason 6e9a661 exists is because commit 76be1fb changes the default file extension for debug symbols from `.mdb` to `.pdb`: How should we know if we're emitting `.mdb` or `.pdb` symbols? *A* way to know that is to actually build everything and see what is generated, but that's...slow. Slow and undesirable. (That's what 6e9a661 does!) An alternate ("hacky") way to know is to rely on mono-specific extensions, and spackle, and duct tape: Mono's `msbuild` sets the `$(_DebugFileExt)` MSBuild property to the file extension of debug symbols, with values of `.mdb` on Mono 4.6 through Mono 4.8, and a value of `.pdb` on Mono 4.9. In `build-tools/scripts/msbuild.mk`, we can probe the mono version and attempt to come up with "sane" defaults, setting `$(_DebugFileExt)` for `xbuild` invocations done via `make`. We can thus rely on the `$(_DebugFileExt)` extension instead of costly filesystem probing, allowing `bundle*.zip` to actually be used, instead of ignored. ~~~ **A tale of two `pkg-config`s**: A funny thing happened while "fixing" `msbuild.mk` to set `$(_DebugFileExt)`: Turns Out™ there are (at least?) *two* `pkg-config` files on the Jenkins+macOS machine: 1. `/usr/local/bin/pkg-config` 2. `/Library/Frameworks/Mono.framework/Commands/pkg-config` (1) knows *nothing* about `Mono.framework`, and we're using the `mono` from `Mono.framework`, which distributes (2). Furthermore, we want to use `pkg-config --atleast-version=4.9 mono` in order to know if we're emitting `.pdb` or `.mdb` debug files, so using the correct version is Very Important. So, off I go to fix that so that we always use (2) on macOS, [and macOS+msbuild breaks][1]. Why's this matter? Recall 0eee673, in which `$(_XAFixMSBuildFrameworkPathOverride)` is set only when (a) `msbuild` is used, and (b) mono prior to 4.8 is used? The check for (b) used the `pkg-config` from (1), meaning *the version check always failed*, meaning when building with `msbuild` from e.g. Mono 4.9, `$(_XAFixMSBuildFrameworkPathOverride)` was still being set, even though, as per 0eee673, it shouldn't have been. Which means the explanation in 0eee673 is wrong. Drain the swamp, at least a little bit: Remove `$(_XAFixMSBuildFrameworkPathOverride)`, and *always* set `$(FrameworkPathOverride)` within `MonoAndroidFramework.props`. It appears that attempting to make `$(FrameworkPathOverride)` conditional wasn't necessary, and only complicates matters. [0]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/312/consoleText [1]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-msbuild-pr-builder/243/
jonpryor
force-pushed
the
jonp-use-bundle-76be1fb19c
branch
from
e962af7 to
449429a
Compare
jonpryor
added a commit
that referenced
this pull request
Nov 8, 2019
Changes: dotnet/java-interop@315968e...47f47bc Context: #3844 * dotnet/java-interop@47f47bc: [generator] Ensure all usage of parameter names are escaped the same (#506) * dotnet/java-interop@dc680d0: [Java.Interop.Tools.JavaCallableWrappers] remove all usage of MD5 (#510)
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 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.
Commit 76be1fb (6e9a661) broke
bundle*.zipuse. Consequently,bundle*.zipis not used, resulting in yuuuge build times(nearly 7 hours for full builds on macOS, over 2 hours for PRs!).
The
_BuildUnlessCachedtarget calls theGetMonoBundleItemstarget, to determine whether the
ForceBuildtarget -- which invokes_BuildRuntimesand many other targets -- should be executed.The
GetMonoBundleItemsreturns all files which would be included inbundle*.zip.The cause of the break is that commit 6e9a661 changed the
GetMonoBundleItemstarget to depend on the_GetMonodocItemstarget, which in turn depended on the
_BuildRuntimestarget._BuildRuntimesis the target we want to avoid executing; it's thetarget that builds mono, and thus can take an eternity (particularly
on the
xamarin-androidJenkins lane, which builds ALL THE ABIS).Adding
_GetMonodocItemsin this fashion thus means we never use thecache, bceause in order to determine whether or not we can use the
cache we need to do all the work that the cache was intended to avoid.
Consider the xamarin-android Build 312 log file
...and BOOM, we're not using
bundle*.zipanymore.The fix is to partially revert commit 6e9a661, but the entire reason
6e9a661 exists is because commit 76be1fb changes the default file
extension for debug symbols from
.mdbto.pdb:How should we know if we're emitting
.mdbor.pdbsymbols?A way to know that is to actually build everything and see what is
generated, but that's...slow. Slow and undesirable. (That's what
6e9a661 does!)
An alternate ("hacky") way to know is to rely on mono-specific
extensions, and spackle, and duct tape: Mono's
msbuildsets the$(_DebugFileExt)MSBuild property to the file extension of debugsymbols, with values of
.mdbon Mono 4.6 through Mono 4.8, and avalue of
.pdbon Mono 4.9.In
build-tools/scripts/msbuild.mk, we can probe the mono version andattempt to come up with "sane" defaults, setting
$(_DebugFileExt)for
xbuildinvocations done viamake.We can thus rely on the
$(_DebugFileExt)extension instead of costlyfilesystem probing, allowing
bundle*.zipto actually be used,instead of ignored.