Skip to content
This repository was archived by the owner on Nov 6, 2024. It is now read-only.

Conversation

@moljac
Copy link
Contributor

@moljac moljac commented Jun 10, 2022

Does this change any of the generated binding API's?

No.

Describe your contribution

Automagic component governance cgmanifest.json generation

@moljac moljac requested a review from jpobst June 10, 2022 15:23
cgmanifest.json Outdated
"NuGetVersion": "120.6.0.1"
}
},
"License": "MIT",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think these licenses are correct, Maven lists it as "Android Software Development Kit License".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea was: "kill em all and let god sort em out".

cgmanifest is there. For those about to rock and check which license is right. Name them and I will assign this PR to them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you're advocating here. We should not be putting in incorrect data. If we cannot create valid data then it is better to omit it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to find a way to fill in the data. This was 1st attempt, so everything is default value for GPS. I could have make it null and make it disappear, or empty string, so there is placeholder for manual update.

Namely making everything completely correct will be near to manual task. Recall Install Referrer which changes licenses on version basis. I came across that case by accident, dumping cgmanifest data like here and comparing. Now, the question is: if we set license and it changes in the future, how do we detect it? License will be incorrect until detected.

Now we have list of components. Some data is currently wrong until I finish workaround for majority of artifacts to fill in License data.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's definitely going to be a challenge (or impossible) to automagically generate correct license data. However we will not be able to publish incorrect license data. I think the default will need to be to omit license data if we cannot determine the correct license.

@@ -0,0 +1,65 @@
#! "net5.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file used? Or is the utilities.cake mechanism used? This file does not exist in the AndroidX version, and we should probably do both repos with the same process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both. I was testing my nugets.

Options do not hurt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like scripting has issues with resolving prerelease packages. Both cake and dotnet script. Nugets are downloaded and resolved, but assemblies are not loaded.

When i published nuget as release everything worked in both cake and csx.

"ArtifactId": "play-services-ads-base",
"GroupId": "com.google.android.gms",
"Version": "20.6.0",
"NuGetId": "Xamarin.GooglePlayServices.Ads.Base",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should NuGetId and NuGetVersion be included? They are not in the existing cgmanifest.json files that are being removed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the docs I could find (https://docs.opensource.microsoft.com/tools/cg/features/cgmanifest), these are not valid values. Additionally, none of the license values are specified. Do you have a more recent doc that describes the structure of the cgmanifest.json file?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should NuGetId and NuGetVersion be included? They are not in the existing cgmanifest.json files that are being removed.

https://github.com/xamarin/XamarinComponents/blob/a89581fd57351fc4fdbdebcf6548cc387f0be995/Android/CoilBase/cgmanifest.json

https://github.com/xamarin/XamarinComponents/blob/b613ea8f070338c575a1587a21d0d78e54ae4ca6/Android/GoogleDagger/cgmanifest.json

you approved

https://github.com/xamarin/XamarinComponents/pull/1025/files

https://github.com/xamarin/XamarinComponents/pull/1025/files

I will stop here.

If you think about it, the purpose of CG is to track buggy/insecure/faulty 3rd party artifacts.

If some artifact+version is bound and delivered/published to our clients/customers it wold be good to know which nuget was that... And that would be NuGetId+Version.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the docs I could find (https://docs.opensource.microsoft.com/tools/cg/features/cgmanifest), these are not valid values. Additionally, none of the license values are specified. Do you have a more recent doc that describes the structure of the cgmanifest.json file?

https://github.com/microsoft/vscode/blob/main/.vscode/cgmanifest.schema.json

Why is it a problem to add NugetId and NugetVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you see here

no one thinks about Maven inside Nuget, so there is no data model for that

marklindsey11/PowerShell@01069f0

maven inside nuget is bindings we do

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you think about it, the purpose of CG is to track buggy/insecure/faulty 3rd party artifacts.

This is true, but this is accomplished by having a contract that standard tools can parse and understand. Adding our own non-standard data to it will not benefit anyone because no tools will be able to understand it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NugetId was redth's idea for improvements (maven to artifact mappings). He added it to several bindings in XamarinComponents repo and he used it in Xamarin.Binding.Helpers. I have no details about the project.

https://github.com/Redth/Xamarin.Binding.Helpers

I found the schema after I generated the code.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it doesn't hurt to add unused data to JSON, so we can do this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed all extras, which were old attempts to solve problems (talked to redth about it).

@moljac moljac merged commit ee3ea3a into main Jun 29, 2022
@moljac moljac deleted the improvements-20220609-cgmanifest branch June 29, 2022 05:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants