Don't optimize URL encoding #1001
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
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60473 In the case when the URL doesn't contain any query elements we optimized for speed and memory by not using UrlBuilder to do the encoding work on the URL. However, it causes problems with URLs that are sent pre-encoded. Remove the optimization and rely on UrlBuilder in all cases.
jonpryor
added a commit
that referenced
this pull request
Jul 8, 2022
Fixes: dotnet/java-interop#335 Fixes: dotnet/java-interop#954 Fixes: dotnet/java-interop#976 Fixes: dotnet/java-interop#981 Fixes: dotnet/java-interop#992 Changes: dotnet/java-interop@7716ae5...c942ab6 * dotnet/java-interop@c942ab68: [java-source-utils] Build one `$(TargetFramework)` (#1007) * dotnet/java-interop@b7caa788: [Byecode] Hide `internal` Kotlin fields marked with `@JvmField` (#1004) * dotnet/java-interop@22d5687b: [generator] Add `@managedOverride` values `none` and `reabstract`. (#1000) * dotnet/java-interop@7f1d2d7a: [generator] Fix <remove-attr/> metadata (#999) * dotnet/java-interop@265ad762: [Java.Interop.Tools.JavaSource] Fix tag parsing errors (#997) * dotnet/java-interop@99c68a86: [Java.Interop] JniTypeSignature & CultureInfo, empty strings (#1002) * dotnet/java-interop@920ea648: [Java.Interop] optimize JniTypeManager.AssertSimpleReference() (#1001) * dotnet/java-interop@4b4fedd3: [generator] Process Javadoc for nested types (#996) Ever since commit 2197a45, CI builds for the `main` branch have been incredibly flakey: 918613d through 27647d2 all failed to complete the **Mac** stage -- which *must* pass in order for most unit tests to run -- then dbadf13 built, then f149c25 failed. Nine commits in the past week have failed to adequately build. (PR builds, meanwhile, were largely fine.) Most of these failures are from `make all-tests`: _ExtractJavadocsFromJavaSourceJars: /Users/runner/android-toolchain/jdk-11/bin/java -jar /Users/runner/work/1/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar @/var/folders/24/8k48jl6d249_n_qfxwsl6xvm0000gn/T/tmp1cf0947e.tmp JAVASOURCEUTILS : warning : Invalid or corrupt jarfile /Users/runner/work/1/s/xamarin-android/bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/Xamarin.Android.McwGen-Tests.csproj] … BINDINGSGENERATOR : warning BG8600: Invalid XML file '/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/obj/Release/javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml': Could not find file "/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/obj/Release/javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml". For details, see verbose output. [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/Xamarin.Android.McwGen-Tests.csproj] System.IO.FileNotFoundException: Could not find file "/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.McwGen-Tests/obj/Release/javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml" … "/Users/runner/work/1/s/xamarin-android/Xamarin.Android-Tests.sln" (default target) (1:2) -> "/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj" (default target) (12:6) -> (CoreCompile target) -> /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(78,37): error CS1061: 'DataEventArgs' does not contain a definition for 'FromNode' and no accessible extension method 'FromNode' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj] /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(79,40): error CS1061: 'DataEventArgs' does not contain a definition for 'FromChannel' and no accessible extension method 'FromChannel' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj] /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(80,40): error CS1061: 'DataEventArgs' does not contain a definition for 'PayloadType' and no accessible extension method 'PayloadType' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj] /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(81,28): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj] /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(82,29): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj] /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(84,51): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj] /Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/BindingTests.cs(85,10): error CS1061: 'DataEventArgs' does not contain a definition for 'Payload' and no accessible extension method 'Payload' accepting a first argument of type 'DataEventArgs' could be found (are you missing a using directive or an assembly reference?) [/Users/runner/work/1/s/xamarin-android/tests/CodeGen-Binding/Xamarin.Android.JcwGen-Tests/Xamarin.Android.JcwGen-Tests.csproj] `java-source-utils.jar` is (apparently) corrupt, so `javadoc-xamarin-test-sources-F6F5170BF7A8BC71.xml` is never created, which means no parameter name information is present, which means the generated `DataEventArgs` type doesn't have the appropriate property names, as the property names come from parameter names, and since we don't have parameter names we instead have property names like `P0`, `P1`, `P2`, etc. Why, then, is `java-source-utils.jar` corrupt? The current guess is that it is being built *concurrently*; from a `msbuild-20220706T175333-leeroy-all.binlog` build log file from one of the offending builds, we see: 17:53:44.526 59:11>Target "_BuildJava: (TargetId:490)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point): 17:53:44.526 59:11>Building target "_BuildJava" completely. Output file "/Users/runner/work/1/s/xamarin-android/bin/Release/lib/packs/Microsoft.Android.Sdk.Darwin/33.0.0/tools/java-source-utils.jar" does not exist. … 17:54:19.564 59:12>Target "DispatchToInnerBuilds: (TargetId:1637)" in file "/Users/runner/work/1/s/xamarin-android/bin/Release/dotnet/sdk/7.0.100-preview.7.22354.2/Microsoft.Common.CrossTargeting.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (target "Build" depends on it): Task "MSBuild" (TaskId:1099) Task Parameter:BuildInParallel=True (TaskId:1099) Task Parameter:Targets=Build (TaskId:1099) Task Parameter: Projects= java-source-utils.csproj AdditionalProperties=TargetFramework=net7.0 (TaskId:1099) Additional Properties for project "java-source-utils.csproj": (TaskId:1099) TargetFramework=net7.0 (TaskId:1099) … 17:54:19.592 59:12>Target "_BuildJava: (TargetId:1640)" in file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.targets" from project "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/tools/java-source-utils/java-source-utils.csproj" (entry point): Building target "_BuildJava" completely. Output file "/Users/runner/work/1/s/xamarin-android/external/Java.Interop/../../bin/Release/lib/xamarin.android/xbuild/Xamarin/Android/java-source-utils.jar" does not exist. … 17:54:41.034 59:11>Done building target "_BuildJava" in project "java-source-utils.csproj".: (TargetId:490) What appears to be happening -- and there is lots of conjecture here, as the build log is enormous and it's difficult to piece everything together -- is that `java-source-utils.csproj` is built *twice*: Once via `Xamarin.Android.sln`, and once via `apksigner.csproj`, as it appears that setting `%(PackageReference.AdditionalProperties)` triggers a *nested build*; note the `DispatchToInnerBuilds` target which invokes the `<MSBuild/>` Task with the specified `AdditionalProperties`. However, that's not the only potential source of concurrent builds; `java-source-utils.csproj` used `$(TargetFrameworks)` (plural), which can *also* result in concurrent builds between the "outer" and "inner" builds used as part of `$(TargetFrameworks)`. Fix this problem by bumping to dotnet/java-interop@c942ab68, which updates `java-source-utils.csproj` to use the singular `$(TargetFramework)` property -- avoiding "outer" and "inner" build problems -- and "for good measure" update `apksigner.csproj` to remove `%(ProjectReference.AdditionalProperties)` on `java-source-utils.csproj`. This should ensure that `java-source-utils.csproj` is only built *once*, which in turn should ensure that `java-source-utils.jar` isn't corrupted, and our CI builds become more reliable
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 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.
Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=60473
In the case when the URL doesn't contain any query elements we
optimized for speed and memory by not using UrlBuilder to do the
encoding work on the URL. However, it causes problems with URLs
that are sent pre-encoded.
Remove the optimization and rely on UrlBuilder in all cases.