[libzip-windows] Fix building with msbuild
#1491
Merged
+24
−11
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
jonpryor
added
the
full-mono-integration-build
Contributor
Author
|
build |
dellis1972
approved these changes
Mar 29, 2018
jonathanpeppers
approved these changes
Mar 29, 2018
grendello
approved these changes
Mar 29, 2018
Contributor
Author
|
The macOS+xbuild PR Build errors should be unrelated to this PR. The macOS+msbuild PR Build timed out (it only had a 6h timeout, not 10h). I have updated the timeout value and restarted the macOS+msbuild PR Build. |
Whenever `external/mono` is bumped, the [`xamarin-android-msbuild` job breaks][0]: [0]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-msbuild/838/ Building target "_Make" completely. ... Task "Touch" (TaskId:1642) Task Parameter: Files= …/bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android/x64/libzip.dll CMake=/Users/builder/android-toolchain/mxe-b9cbb53/bin/x86_64-w64-mingw32.static-cmake CMakeFlags= CopyToOutputDirectory=Always OutputLibrary=x64/libzip.dll OutputLibraryPath=lib/libzip.dll …/bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android/libzip.dll CMake=/Users/builder/android-toolchain/mxe-b9cbb53/bin/i686-w64-mingw32.static-cmake CMakeFlags= CopyToOutputDirectory=Always OutputLibrary=libzip.dll OutputLibraryPath=lib/libzip.dll (TaskId:1642) Touching "…/bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android/x64/libzip.dll". (TaskId:1642) …/build-tools/libzip/libzip.targets(52,5): error MSB3375: The file "…/bin/Debug/lib/xamarin.android/xbuild/Xamarin/Android/libzip.dll" does not exist. […/build-tools/libzip-windows/libzip-windows.csproj] Done executing task "Touch" -- FAILED. (TaskId:1642) This is due to an "`xbuild`-ism" we were inadvertently using which `msbuild` doesn't like: when item metadata is used in `//Target/@Inputs`, [MSBuild Target Batching][1] is *not* used. [1]: https://msdn.microsoft.com/en-us/library/ms171473.aspx Case in point, the `_Make` target: <!-- BAD! --> <Target Name="_Make" Condition=" '@(_LibZipTarget)' != '' " Inputs="$(IntermediateOutputPath)\%(_LibZipTarget.Identity)\Makefile" Outputs="@(Content)"> Because this used "inline" item metadata, the target wasn't being batched as intended (and as `xbuild` does). Consequently, `@(_LibZipTarget)` was only being built for mxe-Win64, *not* mxe-Win32, which is why the `<Touch/>` use within the `_Make` target failed. The fix? Don't Do That™. Instead, use a new `@(_LibZipTargetMakefile)` item group for the `//Target/@Inputs`, which allows `msbuild` to properly batch the `_Make` target contents: <ItemGroup> <_LibZipTargetMakefile Include="$(IntermediateOutputPath)\%(_LibZipTarget.Identity)\Makefile" /> </ItemGroup> <Target Name="_Make" Condition=" '@(_LibZipTarget)' != '' " Inputs="@(_LibZipTargetMakefile)" Outputs="@(Content)"> Review other files matching `git grep 'Inputs=.*%'` and fix them too.
jonpryor
force-pushed
the
jonp-msbuild-libzip-windows
branch
from
596a08c to
a8c3c72
Compare
Contributor
Author
|
Current macOS+xbuild PR Build error: "Disk full." :-( Considering that it previously built, and that it now builds on macOS+msbuild (which was the primary point to this PR), I'm more than happy to merge this. |
akoeplinger
pushed a commit
to akoeplinger/xamarin-android
that referenced
this pull request
Apr 4, 2018
Commit 9d131af introduced a [unit test failure][m573] on xamarin-android/master: [m573]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android/573/ Xamarin.Android.Build.Tests.BuildTest.XA4212 Expected: String containing "warning XA4" But was: <log file contents...> Aside: The XA4212 test creates a project with a bad `IJavaObject` type, builds the project -- which triggers an XA4212 error, which *is* observed in the unit test -- then *rebuilds* the project, but this time with `$(AndroidErrorOnCustomJavaObject)`=False. The intention is that the second build should instead elicit an XA4212 *warning*, but otherwise continue just fine. What's interesting about the log file contents is that it shows that the `<GenerateJavaStubs/>` task isn't executed *at all*, but it's the `<GenerateJavaStubs/>` task which is supposed to emit the warning! Skipping target "_GenerateJavaStubs" because its outputs are up-to-date. Update the `<GenerateJavaStubs/>` task so that if it errors out, it *removes* any `<GenerateJavaStubs/>` output files. This will ensure that on subsequent builds, the `_GenerateJavaStubs` target won't be inadvertently skipped. (This issue *should* have been caught during the [**macOS+xbuild PR builder** build for PR dotnet#797][pr1491]. It *wasn't*, because the `Xamarin.Android.Build.Tests` tests weren't reported. For example, PR dotnet#1491 ran 869 tests, while PR dotnet#1492 ran 1085! I'm not sure what can be done to better detect and prevent this in the future.) [pr1491]: https://jenkins.mono-project.com/view/Xamarin.Android/job/xamarin-android-pr-builder/1491/
Contributor
|
This change broke build caching. Now api-*.xml.in is generated EVERY TIME, which adds tremendous aount of build time. And this build happens every time anything that depends on api-*.xml.in and all its subsequent dependencies. For example, Xamarin.Android.Build.Tasks build is suffered from it. |
Contributor
|
This is why things broke. Basically it is due to hacky use of generated item list without existence verification. Target "_ClassParse" has Inputs="@(_ApiParameterDescription)" which is expanded as follows: Note that many of those (e.g. API Level 4-9) do not exist. For non-existent inputs, MSBuild targets always execute. |
Contributor
|
Another problem with this condition: if I touch api-27.params.txt it rebuilds everything, not just api-27. |
atsushieno
added a commit
to atsushieno/xamarin-android
that referenced
this pull request
Apr 6, 2018
dotnet#1491 broke build caching. Now api-*.xml.in is generated EVERY TIME, which adds tremendous amount of build time. And this build happens every time anything that depends on api-*.xml.in and all its subsequent dependencies. For example, Xamarin.Android.Build.Tasks build is suffered from it. The cause of the problem is this: Inputs="@(_ApiParameterDescription)" which is expanded as follows: /sources/xamarin-android/build-tools/api-xml-adjuster/../../src/Mono.Android/Profiles/api-4.params.txt;/sources/xamarin-android/build-tools/api-xml-adjuster/../../src/Mono.Android/Profiles/api-5.params.txt; ... Note that many of those (e.g. API Level 4-9) do not exist. For non-existent inputs, MSBuild targets always execute. To fix the issue, add another build task that returns only existent files and use it to determine the actual targets. This is sadly a compromised solution that updates on ANY API level will trigger generation of ALL API levels (but must be applied IMMEDIATELY without excuse for "better solution". It's a BLOCKER for development. No one can work on MSBuild tasks if this bug remains).
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 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.
Whenever
external/monois bumped, thexamarin-android-msbuildjob breaks:This is due to an "
xbuild-ism" we were inadvertently using whichmsbuilddoesn't like: when item metadata is used in//Target/@Inputs, MSBuild Target Batching is not used.Case in point, the
_Maketarget:Because this used "inline" item metadata, the target wasn't being
batched as intended (and as
xbuilddoes). Consequently,@(_LibZipTarget)was only being built for mxe-Win64, notmxe-Win32, which is why the
<Touch/>use within the_Maketargetfailed.
The fix? Don't Do That™. Instead, use a new
@(_LibZipTargetMakefile)item group for the
//Target/@Inputs, which allowsmsbuildtoproperly batch the
_Maketarget contents:Review other files matching
git grep 'Inputs=.*%'and fix them too.