cake-contrib / Cake.AddinDiscoverer

Tool to aid with discovering information about Cake Addins
MIT License
5 stars 6 forks source link

Incorrect repository URL derived from projectUrl #151

Closed augustoproiete closed 3 years ago

augustoproiete commented 3 years ago

It seems there's a bug in the logic that reads the projectUrl (and/or repositoryUrl) from the NuGet package which is appending a / (forward slash) at the end of the URL, causing some URLs to become invalid.

One example would be Cake.Xamarin.Binding.Util.yml which was assigned Repository to https://go.microsoft.com/fwlink/?linkid=864959/ (notice the slash at the end) when the NuGet spec doesn't have that slash at the end:

Cake.Xamarin.Binding.Util.nuspec:

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    <id>Cake.Xamarin.Binding.Util</id>
    <version>2.0.0</version>
    <title>Cake Add-In providing utilities for Xamarin Binding build scripts</title>
    <authors>Microsoft</authors>
    <owners>Microsoft</owners>
    <requireLicenseAcceptance>false</requireLicenseAcceptance>
    <licenseUrl>https://go.microsoft.com/fwlink/?linkid=865029</licenseUrl>
    <projectUrl>https://go.microsoft.com/fwlink/?linkid=864959</projectUrl>
    <description>Cake Add-In providing utilities for Xamarin Binding build scripts</description>
    <copyright>© Microsoft Corporation. All rights reserved.</copyright>
    <dependencies>
      <dependency id="Mono.Cecil" version="0.10.0" />
    </dependencies>
  </metadata>
</package>

This causes the link in the Cake website not to work (for this particular add-in at least):

Cake.Xamarin.Binding.Util repository link


Relates to https://github.com/cake-build/website/issues/1195

pascalberger commented 3 years ago

I wouldn't implement something special to handle the Xamarin case, but instead implement separate attributes for Repo URL and Project URL (https://github.com/cake-build/website/issues/1195) which Addin Discoverer can pick form NuGet packages (since they can contain repo metadata nowadays)

Jericho commented 3 years ago

It seems there's a bug in the logic that reads the projectUrl (and/or repositoryUrl) from the NuGet package which is appending a / (forward slash) at the end of the URL, causing some URLs to become invalid.

Quoting @agc93 This is always a hot topic.

Adding the final slash at the end of URLs was deliberate. It was debated and we agreed to standardize URLs to be consistent. I realize you have found one edge scenario where it doesn't work but the logic we have implemented works well for all other addins (as far as I know).

By the way, why is the URL for this addin pointing to Microsoft's web site? To me, it seems like the issue appears to be an incorrect URL in the package metadata rather than a flaw in the AddinDiscoverer logic.

For all these reasons, I am not inclined to change the AddinDisco logic.

but instead implement separate attributes for Repo URL and Project URL

FYI, AddinDisco already tracks repo URL and project URL separately. Sometimes one URL is missing so we use the other one as a fallback. Currently only the repo URL is included in the YAML content but let me know if you would like both of these URLs to be included.

augustoproiete commented 3 years ago

@Jericho @agc93 I might be missing something, but as a matter of principle shouldn't the AddinDisco (and the website) honor the URL the addin package author intended to be used (and purposefully defined in the NuGet metadata), without making any changes to it?

By the way, why is the URL for this addin pointing to Microsoft's web site? To me, it seems like the issue appears to be an incorrect URL in the package metadata rather than a flaw in the AddinDiscoverer logic.

It's a redirect URL https://go.microsoft.com/fwlink/?linkid=864959 that sends the user to the GitHub repo. AFAIK it is one of the ways Microsoft has to ensure permanent links to resources. I suspect they also use it for tracking clicks.

Jericho commented 3 years ago

Sure, in theory it would be great to blindly accept the URL from the package metadata. But theory and reality are two very different things, unfortunately! Over the years we found several problems with these URL such as sometimes they are missing, sometimes they point to an original location despite the fact that the addin had been transferred, sometimes they use the HTTP scheme when HTTPS is in fact required, lack of standardization, etc. (these are just a few examples I can remember off the top of my head). We tried our best to solve these problems with thoughtful logic over several iterations during the last 2 years.

Even more frustrating: I submitted several PRs to fix some of these issues in the addin repos but authors don't care/don't have time/don't think it's important/can't be bothered and the PRs are never merged and the problems never resolved. I appreciate your principled approach but unfortunately we can't ignore all these issues.

You found one edge case. I acknowledge that, no question about it but it doesn't mean that the whole logic should be rejected "as a matter of principle". As the saying goes: "don't throw the baby out with the bathwater"!

It's probably worth trying to find a way to improve our logic so it can handle the edge case you found rather than get rid of the logic altogether.

Jericho commented 3 years ago

From now on the standardization logic will only apply to GitHub URIs which is probably 90%, if not more, of the project URLs. This will leave URLs such as "https://go.microsoft.com/fwlink/?linkid=864959" unchanged.