[generator] strip fields and methods that have '$' in the name. #116
Conversation
|
Tests? :-) Would probably be sufficient to add a field containing |
|
That sounds good, but the tests never run... |
|
Actually they are gone once I ran "make" at top level (which seems to be required apart from our product build). But I don't see any relevant code generation for my api.xml additions. |
Isn't that the point? ;-) If you're adding fields or methods containing Aside: it's the |
|
I have an additional test that should generate a class without method (that have invalid name), and that was not generated either. |
What was the test, exactly? Can you provide a gist/etc.? For example, if I apply this patch: diff --git a/tools/generator/Tests-Core/api.xml b/tools/generator/Tests-Core/api.xml
index b62bd74..c1c7bfe 100644
--- a/tools/generator/Tests-Core/api.xml
+++ b/tools/generator/Tests-Core/api.xml
@@ -24,6 +24,8 @@
<parameter name="p0" type="java.lang.Object">
</parameter>
</method>
+ <method abstract="false" deprecated="not deprecated" final="false" name="getSpanFlags$IgnoreMe" native="false" return="int" static="false" synchronized="false" visibility="public">
+ </method>
</class>
<class abstract="false" deprecated="not deprecated" extends="android.text.SpannableStringInternal" extends-generic-aware="android.text.SpannableStringInternal" final="false" name="SpannableString" static="false" visibility="public">
<implements name="android.text.Spannable" name-generic-aware="android.text.Spannable">
@@ -47,6 +49,8 @@
</method>
<field deprecated="not deprecated" final="true" name="SPAN_COMPOSING" static="true" transient="false" type="int" type-generic-aware="int" value="256" visibility="public" volatile="false">
</field>
+ <field deprecated="not deprecated" final="true" name="SPAN_COMPOSING$IGNORE_ME" static="true" transient="false" type="int" type-generic-aware="int" value="256" visibility="public" volatile="false">
+ </field>
</interface>
</package>
<package name="java.lang">Then diff -rup tools/generator/Tests-Core/expected/Android.Text.ISpanned.cs bin/TestDebug/generator-core/Android.Text.ISpanned.cs
--- tools/generator/Tests-Core/expected/Android.Text.ISpanned.cs 2016-05-30 21:26:11.000000000 -0400
+++ bin/TestDebug/generator-core/Android.Text.ISpanned.cs 2017-01-26 10:51:04.000000000 -0500
@@ -4,6 +4,27 @@ using Android.Runtime;
namespace Android.Text {
+ [Register ("android/text/Spanned", DoNotGenerateAcw=true)]
+ public abstract class Spanned : Java.Lang.Object {
+
+ internal Spanned ()
+ {
+ }
+
+ // Metadata.xml XPath field reference: path="/api/package[@name='android.text']/interface[@name='Spanned']/field[@name='SPAN_COMPOSING$IGNORE_ME']"
+ [Register ("SPAN_COMPOSING$IGNORE_ME")]
+ public const int SpanComposing$ignoreMe = (int) 256;
+ }
+
+ [Register ("android/text/Spanned", DoNotGenerateAcw=true)]
+ [global::System.Obsolete ("Use the 'Spanned' type. This type will be removed in a future release.")]
+ public abstract class SpannedConsts : Spanned {
+
+ private SpannedConsts ()
+ {
+ }
+ }
+
// Metadata.xml XPath interface reference: path="/api/package[@name='android.text']/interface[@name='Spanned']"
[Register ("android/text/Spanned", "", "Android.Text.ISpannedInvoker")]
public partial interface ISpanned : IJavaObject {
... |
|
It never worked as in that the unit tests never reported anyhing (as I implied I was using XS to run tests). It seems that the test build gives warnings on some missing attributes (my was missing 'extends' attribute) and then nothing was generated. After fixing that it reports mismatches as expected. However the your test command never worked as expected, like, even if I insert "throw new Exception();" in the middle of Adjuster.Proess() in ApiXmlAdjuster.cs it never threw anything. That means, it does not use generator.exe that the build system built. It seems that make target is hacky and some overhaul is needed on the overcomplicated build system. |
atsushieno
force-pushed
the
api-xml-adjuster-strip-non-bindables
branch
from
8c65fa3 to
8c82e18
Compare
|
It seems that your mentioned tests do not actually test what I expect because the suggested API XML is input to generator.exe without --class-parse, which is not applicable here. ApiXmlAdjuster is used to cover class-parse defects, and my fix is exactly about that. Therefore generator.exe run for my test requires --class-parse option, but that's not what these existing tests should actually test. In short, adding itms in this api.xml doesn't work. |
atsushieno
force-pushed
the
api-xml-adjuster-strip-non-bindables
branch
from
8c82e18 to
aa0349d
Compare
|
Everything is reverted to the initial PR changes. |
atsushieno
force-pushed
the
api-xml-adjuster-strip-non-bindables
branch
from
aa0349d to
f9549bf
Compare
|
The "problem" is that Thus, an alternative suggestion: add a new NUnit-based test into diff --git a/tools/generator/Tests/expected/StaticMethods/StaticMethod.xml b/tools/generator/Tests/expected/StaticMethods/StaticMethod.xml
index 913f5e6..775f513 100644
--- a/tools/generator/Tests/expected/StaticMethods/StaticMethod.xml
+++ b/tools/generator/Tests/expected/StaticMethods/StaticMethod.xml
@@ -1,5 +1,5 @@
<U+FEFF><?xml version="1.0" encoding="UTF-8" ?>
-<api>
+<api api-source="class-parse">
<package name="java.lang">
<class abstract="false" deprecated="not deprecated" final="false" name="Object" static="false" visibility="public">
</class>
@@ -13,6 +13,8 @@
</method>
<method abstract="false" deprecated="Deprecated please use methodAsString" final="false" name="Obsoletemethod" native="false" return="java.lang.String" static="true" synchronized="false" visibility="public">
</method>
+ <method abstract="false" deprecated="not deprecated" final="false" name="getSpanFlags$IgnoreMe" native="false" return="int" static="false" synchronized="false" visibility="public">
+ </method>
</class>
</package>
</api>Then build and run the tests: $ (cd tools/generator/Tests; xbuild)
$ make run-tests TESTS=bin/TestDebug/generator-Tests.dllInterestingly, this actually dies with a Perhaps it fails because additional parameters are required by
|
|
I don't see that crash locally. What I'm seeing is more problematic, conceptual problem on your suggestion... Under the |
atsushieno
force-pushed
the
api-xml-adjuster-strip-non-bindables
branch
from
f9549bf to
2fd53f0
Compare
|
New tests are added in almost the same manner. |
| static IntPtr INVALID_$FIELD_jfieldId; | ||
|
|
||
| // Metadata.xml XPath field reference: path="/api/package[@name='xamarin.test.invalidnames']/class[@name='InvalidNameMembers']/field[@name='INVALID_$FIELD']" | ||
| [Register ("INVALID_$FIELD")] |
There was a problem hiding this comment.
Perhaps I'm deeply confused, but I thought the idea was that if api.xml has /api[@apisource='class-parse'], then members with $ would be skipped.
So why is INVALID$FIELD being bound? Shouldn't this member not be present?
There was a problem hiding this comment.
huh, yeah it doesn't make sense. Why does tests pass even with this? The answer is most likely because "unless I run 'make clean' it does not run tests against the binaries I just built"...
Related: https://bugzilla.xamarin.com/show_bug.cgi?id=46344#c12 class-parse has brought another problem; it generates fields and methods that are compiler-generated and should not have been written to XML in the first place. class-parse has no idea on how to deal with them, so kill them in api-xml-adjuster instead.
atsushieno
force-pushed
the
api-xml-adjuster-strip-non-bindables
branch
from
2fd53f0 to
1c1f6dc
Compare
Related: https://bugzilla.xamarin.com/show_bug.cgi?id=46344#c12 class-parse has brought another problem; it generates fields and methods that are compiler-generated and should not have been written to XML in the first place. class-parse has no idea on how to deal with them, so kill them in api-xml-adjuster instead.
Changes: dotnet/android-tools@d92fc3e...c5732a0 * dotnet/android-tools@c5732a0: [Xamarin.Android.Tools.AndroidSdk] Support Microsoft Dist JDK (#117) * dotnet/android-tools@52ef989: [Xamarin.Android.Tools.AndroidSdk] Fix CS0168 warning (#116)
Context: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1314263 Context: c936d09 Changes: dotnet/android-tools@d92fc3e...34e98e2 * dotnet/android-tools@34e98e2: [build] Allow Assembly "vendorization" (#136) * dotnet/android-tools@061bcc2: [build] Import "parent directory.override.targets" (#135) * dotnet/android-tools@a5194e9: [Xamarin.Android.Tools.AndroidSdk] Downgrade build-tools to API-30 * dotnet/android-tools@440e6be: [Xamarin.Android.Tools.AndroidSdk] Update SDK component for API-31 (#134) * dotnet/android-tools@9b658b2: Merge pull request #133 from xamarin/ndk-r23 * dotnet/android-tools@ff73f92: [build] Use GitInfo to generate $(Version) (#131) * dotnet/android-tools@4c2e36c: [Xamarin.Android.Tools.AndroidSdk] Eclipse Adoptium support (#132) * dotnet/android-tools@eaec4e3: [Xamarin.Android.Tools.AndroidSdk] More Microsoft Dist JDK Support (#130) * dotnet/android-tools@f9c1b0d: [BaseTasks] improve Task settings in AsyncTaskExtensions (#129) * dotnet/android-tools@efc9b67: Merge pull request #128 from dellis1972/bumpitybump * dotnet/android-tools@40adec0: Bump LibZipSharp and Mono.Unix to latest stable versions, * dotnet/android-tools@87acd6b: [Microsoft.Android.Build.BaseTasks] add StrongName (#127) * dotnet/android-tools@02f7ae7: [NDK] Properly detect 64-bit NDK * dotnet/android-tools@623332d: Bump LibZipSharp to 2.0.0-alpha7 (#126) * dotnet/android-tools@c055fa8: [Microsoft.Android.Build] Bump to MSBuild 16.10.0 (#125) * dotnet/android-tools@49936d6: Merge pull request #124 from xamarin/update-libzipsharp * dotnet/android-tools@ef78dfc: Bump LibZipSharp to 2.0.0-alpha6 * dotnet/android-tools@e3d708c: [BaseTasks] fix `\`-delimited paths on macOS (#122) * dotnet/android-tools@bdcf899: Reference the new Mono.Unix nuget (#123) * dotnet/android-tools@90d7621: [BaseTasks] add ABI detection for RIDs (#121) * dotnet/android-tools@79e3b97: [JdkInfo] handle invalid XML from /usr/libexec/java_home (#120) * dotnet/android-tools@81519fe: Add SECURITY.md (#119) * dotnet/android-tools@683f375: [Microsoft.Android.Build] Use MSBuild NuGets (#118) * dotnet/android-tools@2241489: [Xamarin.Android.Tools.AndroidSdk] Support Microsoft Dist JDK (#117) * dotnet/android-tools@52ef989: [Xamarin.Android.Tools.AndroidSdk] Fix CS0168 warning (#116) Moving the Roslyn Analyzers initialization code from `Directory.Build.props` to `Directory.Build.targets` (c936d09) caused the Roslyn Analyzers to be applied to code imported from `external/xamarin-android-tools`. Previously the xamarin-android-tools code was unaffected because it had its own `Directory.Build.props` file, so it did not traverse the file system upwards any further to find the Java.Interop files. Fixes for Roslyn issues has not been applied to `xamarin-android-tools` code, so it adds ~150 warnings to the Java.Interop build. dotnet/android-tools@ff73f925 added a `Directory.Build.targets`, so updating `external/xamarin-android-tools` to the latest xamarin-android-tools commit will prevent Java.Interop's `Directory.Build.targets` file from being used, which will exclude xamarin-android-tools from the Roslyn Analyzers.
Related: https://bugzilla.xamarin.com/show_bug.cgi?id=46344#c12
class-parse has brought another problem; it generates fields and methods
that are compiler-generated and should not have been written to XML
in the first place.
class-parse has no idea on how to deal with them, so kill them in
api-xml-adjuster instead.