-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
installreferrer bindings #556
Conversation
source/com.android.installreferrer/installreferrer/Transforms/Metadata.Namespaces.xml
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
config.json
Outdated
"artifactId": "installreferrer", | ||
"version": "1.0", | ||
"nugetVersion": "1.0.0", | ||
"nugetId": "Xamarin.Google.Android.Installreferrer", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalization: InstallReferrer
@@ -0,0 +1,27 @@ | |||
<metadata> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With #555 committed, this file should be deleted, and replaced with the following lines in Directory.Build.props
:
<AndroidNamespaceReplacement Include='com.android.' Replacement='Xamarin.Android' />
com.google.android.finsky.externalreferrer
-> Xamarin.Google.Android.Finsky.ExternalReferrer
- This one is a little more difficult because our other libraries do not append Xamarin
before Google
for the `com.google.android' namespace. For example:
Google.Android.Material.Animation
If we want to use Xamarin.Google.Android
, we will need to add:
<AndroidNamespaceReplacement Include='com.google.android.' Replacement='Xamarin.Google.Android' />
It will need to be added before this existing line so that it is run first:
<AndroidNamespaceReplacement Include='com.google.' Replacement='Google' />
I can support it either way:
Google.Android.Finsky.ExternalReferrer
to be consistent with MaterialXamarin.Google.Android.Finsky.ExternalReferrer
because we really should haveXamarin
in our namespaces so it doesn't conflict with anyone else's packages
🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I not allowed to change namespaces "old way" (with metadata)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, the new way is less work. It would require no work in most new cases. This particular one requires 2 lines the way as opposed to 4 lines the old way.
The new way also runs faster in generator
, making builds faster.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now, just for changing namespaces we have 2 ways of doing it
- XPath/XML
- MSBuild
In a spirit of "growth mindset" let us further grow number of options to fix namespaces with:
- Roslyn analyzers
- Source generators
Just to make bindings more approachable for users and technology acceptance. Bindings are trivial and everybody does it just for fun and users are thrilled with alternatives. The more options the better and users will benefit by being forced to learn, just they don't know that. Options never raise questions and express freedom and inclusiveness.
Pro argument for MsBuild approach I recently heard: "new (MSBuild )way is centrally managed".
We have central metadata files in both AX and GPS-FB-MLKit which could be used for central management of namespaces.
Now, performance. I accept that MsBuild approach might be faster than XPath, but we have other problems, like hundreds if not thousands of matched no node
which slow down builds, etc etc.
For the record:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little bit if the PR is not approved for using "other way" or "old way".
|
||
|
||
|
||
Apache License |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This library does not appear to be licensed under Apache 2.0: https://maven.google.com/web/index.html?q=g#com.android.installreferrer:installreferrer:2.2
It appears to be licensed under the Android Software Development Kit License. I suspect that this license does not allow us to redistribute the .aar
file, and we will need to use Xamarin.Build.Download
for this package:
Except to the extent required by applicable third party licenses, you may not copy (except for backup purposes), modify, adapt, redistribute, decompile, reverse engineer, disassemble, or create derivative works of the SDK or any part of the SDK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version 1.0 is under Apache.
They changed it in version 1.1.
Let us bind and release version 1.0, so Facebook components are unblocked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran over this issue (license varying across versions) during cgmanifest.json
generation. I came up with some sort of solution, but writing tons of if-then-else will be nearly manual job, which is also not desireable.
templates/installreferrer/License.md
Outdated
@@ -0,0 +1,15 @@ | |||
**Xamarin is not responsible for, nor does it grant any licenses to, third-party packages. Some packages may require or install dependencies which are governed by additional licenses.** | |||
|
|||
Note: This component depends on [com.android.installreferrer:installrefererrer](https://maven.google.com/web/index.html?q=installreferrer#com.android.installreferrer:installreferrer), which is subject to the [Apache 2.0](https://developer.android.com/google/play/installreferrer/) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!Apache 2.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
version 1.0 is Apache 2.0
https://maven.google.com/web/index.html?q=install#com.android.installreferrer:installreferrer:1.0
<Authors>Microsoft</Authors> | ||
<Owners>Microsoft</Owners> | ||
<Copyright>© Microsoft Corporation. All rights reserved.</Copyright> | ||
<PackageProjectUrl>https://go.microsoft.com/fwlink/?linkid=2091414</PackageProjectUrl> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Remove
<PackageProjectUrl>
/<PackageRequireLicenseAcceptance>
so it picks up the ones inDirectory.Build.props
. - There likely isn't a
<PackageLicenseExpression>
for this license, since it isn't open source. We should use<PackageLicense>
instead of the deprecated<PackageLicenseUrl>
. (https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#packing-a-license-expression-or-a-license-file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<None Include="..\..\templates\auto-value\License.md" Pack="true" PackagePath="License.md" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!auto-value
utilities.cake
Outdated
@@ -214,6 +214,7 @@ Task ("spell-check") | |||
"J2Objc", | |||
"CheckerFramework", | |||
"CheckerQual", | |||
"Installreferrer", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalization: InstallReferrer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
…rin/AndroidX into mu-20220518-installreferrer
well not anymore - 16 |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
samples build error ACW generation (note net6 only):
MCWs:
Binlog: |
proguard rules do not help:
|
azure-pipelines.yml
Outdated
XamarinDotNetWorkloadSource: https://aka.ms/dotnet/maui/rc.3.json | ||
XamarinDotNetWorkloadSource: https://aka.ms/dotnet/maui/6.0.400.json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to use the local file below? But can you update .NET & workloads in a separate PR? Just to make sure that doesn't break anything on its own.
Does this change any of the generated binding API's?
Yes - new bindings.
extracted from: #468
Describe your contribution
com.android.installreferrer:installreferrer
- -> 1.0