Feature: GitWeb Source Link provider #505

Glen-Nicol-Garmin · 2019-11-25T17:57:40Z

Hi there, I work for Garmin and we use an internal GitWeb server that doesn't work with any of the existing providers. I created one that should work for many other companies in a similar situation that don't use cloud providers.

I saw in #239 that the suggested solution for a provider that is not currently supported is to copy and edit the GitLab ones. So that is what I did.

dnfclas · 2019-11-25T17:57:53Z

All CLA requirements met.

tmat · 2019-11-25T21:55:40Z

Cools. Thanks for the PR, much appreciated! We are currently working on releasing first stable version of SourceLink packages, so I'm gonna wait with merging this until after we finish that process.

Glen-Nicol-Garmin · 2019-11-26T01:17:23Z

Sounds good. Is there an estimate on how long that will take?

tmat · 2019-11-26T01:30:42Z

A couple of days :)

src/SourceLink.GitWeb/GetSourceLinkUrl.cs

src/SourceLink.GitWeb/build/SourceLink.GitWeb.targets

src/SourceLink.GitWeb/buildMultiTargeting/SourceLink.GitWeb.targets

src/SourceLink.GitWeb/Microsoft.SourceLink.GitWeb.nuspec

src/SourceLink.GitWeb/GetSourceLinkUrl.cs

tmat

Looks good, just a couple of issues.
It'd also be useful to have integration tests - they validate that the package works end-to-end. See https://github.com/dotnet/sourcelink/blob/master/src/SourceLink.Git.IntegrationTests/GitLabTests.cs

dotnet#505

Glen-Nicol-Garmin · 2019-12-02T16:59:06Z

I'm looking into integration tests now. Will be a separate commit than the above review comments.

Edit:
This is likely a dumb question, what is the purpose of these MSBuild Properties?

"$(PrivateRepositoryUrl)"
"$(RepositoryUrl)

Furthermore, at least in our case, we always use the ssh URL for git operations(not sure if gitweb supports anything but ssh?). So should I also be modifying TranslateRepositoryUrls to keep the ssh url in those properties? I understand source link needs an HTTP address for the content url.

tmat · 2019-12-27T09:02:14Z

@Glen-Nicol-Garmin Will you be able to write the integration test? I can help if needed.

Re PrivateRepositoryUrl - See https://github.com/dotnet/sourcelink/tree/master/docs#publishrepositoryurl

tmat · 2019-12-27T09:06:40Z

Re SSH - the default implementation is provided by TranslateRepositoryUrlsGitTask, from which TranslateRepositoryUrls derives. If the default implementation works then TranslateRepositoryUrls can be empty. AzureDevOpsServer uses a different structure for SSH URL than it uses for HTTP URL, so it needs to override:

https://github.com/dotnet/sourcelink/blob/master/src/SourceLink.AzureDevOpsServer.Git/TranslateRepositoryUrls.cs#L14

Glen-Nicol-Garmin · 2019-12-30T17:00:06Z

I can do the work for the tests but I am still a bit confused on how those properties should be set. Sounds like in this case they should both be left as SSH urls? I gather that because from my interpretation those properties are supposed to be the URL for the source control software to do a clone/checkout and gitweb only does SSH (as far as I can tell).

tmat · 2020-01-01T18:25:28Z

Sounds like in this case they should both be left as SSH urls? I gather that because from my interpretation those properties are supposed to be the URL for the source control software to do a clone/checkout and gitweb only does SSH (as far as I can tell).

They need to be translated to https URLs. RepositoryUrl is embedded in a .nuspec by NuGet pack task. See https://github.com/dotnet/sourcelink/blob/master/src/SourceLink.Git.IntegrationTests/GitHubTests.cs#L267

Glen-Nicol-Garmin · 2020-01-02T20:41:49Z

When I read the nuspec docs I interpret that property as the clone URL.

This should be a publicly available url that can be invoked directly by a version control software.

Which in our case is the SSH URL. We can't clone from a https URL. If SourceLink requires it to be a HTTPS url then fine, but we should add documentation for the divergence.

tmat · 2020-01-02T21:33:48Z

We can't clone from a https URL

Can you elaborate on why it is not possible to clone using https URL?

Glen-Nicol-Garmin · 2020-01-02T22:16:31Z

Our git server only supports the SSH protocol that I am aware of. When you go to a gitweb page it lists the SSH url to clone it.

tmat · 2020-01-02T22:31:56Z

That seems odd. Have you tried the https version of the URI? Source Link needs HTTPs support for at least fetching individual raw source files. It would be surprising if you could do that but couldn't clone the repo using HTTPs.

Glen-Nicol-Garmin · 2020-01-02T22:37:52Z

I thought the point of source link was to translate between repo Urls to content urls where the files can be found and downloaded? We have content over HTTPS.

And no simply changing the protocol of the URI does not work.

tmat · 2020-01-02T23:20:01Z

I thought the point of source link was to translate between repo Urls to content urls where the files can be found and downloaded? We have content over HTTPS.

That is correct. It's just surprising that you wouldn't have HTTPS URL that would work for clone. I have not seen that before -- GitHub, Azure DevOps, GitLab, BitBucket all allow both HTTPS and SSH (and also git protocol). I guess GitWeb can be configured to only support SSH?

If that's the case we can either skip the translation completely by not setting SourceControlManagerUrlTranslationTargets in the GitWeb targets file and removing the translation task, or overriding some of the methods on the translation task class to not translate ssh to https.

tmat · 2020-01-03T00:00:12Z

BTW, the reason we translate SSH and git protocols to HTTPS is to normalize the build results. You can imagine different build environments choosing different protocols for cloning the repo. The result of the build from all these environments will be the same, regardless of how they cloned the repo.

tmat · 2020-01-03T16:43:30Z

How about merging this PR and leaving the integration tests and SSH translation for a follow up?

Glen-Nicol-Garmin · 2020-01-03T17:00:22Z

I think I need to wrap my head around the SSH translation before I really want to consider it done and ready for consumption. I should be able to get to that next week.

tmat · 2020-01-03T17:25:49Z

BTW, I'm going to merge #546 which might cause your PR to fail build due to missing nullable annotations. Should be easy to add though.

Glen-Nicol-Garmin · 2020-01-06T18:32:57Z

So funny story, I tried removing the translation step altogether (commented out the using Task, the property you mentioned, and the target) still got https URLS. And then I tried overriding all those functions in TranslateRepositoryUrlsGitTask. Neither works in the integration tests. I am thinking I may not have something setup right for those target files.

Glen-Nicol-Garmin · 2020-01-08T23:04:24Z

Any leads or ideas on why changing those files isn't affecting the integration tests?

Glen-Nicol-Garmin · 2020-01-30T23:24:50Z

I was able to figure it out. Did not know the integration tests rely on the nuget packages which are not built when you build the solution.

tmat · 2020-01-30T23:57:13Z

Oh, sorry about that! Yes, the build doesn't run pack by default. You need to run build -pack from command line. I think it would make sense to build the packages by default. It would be easier to work with.

Glen-Nicol-Garmin · 2020-01-30T23:59:35Z

No worries, would you like me to rebase onto master before you review the latest changes?

tmat · 2020-01-31T00:03:41Z

@Glen-Nicol-Garmin Yes, please rebase.

GitWeb only currently supports SSH repository URLs. dotnet#505

Glen-Nicol-Garmin · 2020-01-31T00:14:52Z

K, done. You should look at the changes I made to the common translation Task to facilitate checking for supported protocols.

src/Common/TranslateRepositoryUrlGitTask.cs

tmat

🕐

GitWeb only currently supports SSH repository URLs. dotnet#505

Glen-Nicol-Garmin · 2020-02-06T21:52:24Z

Also, as an aside maybe this belongs in a separate issue, whenever I run the tests an entry like this is added to my user PATH variable:

C:\Users\<username>\AppData\Local\Temp\SourceLinkTests\40f584a7-8ac8-427e-a078-05cd8fe22c08\.home\.dotnet\tools

These are never cleaned up. And it actually ended up breaking a few things on my system in a subtle way.

For instance, my user TEMP directory wouldn't get set so all my programs would try to write to the system TEMP directory at C:\windows\TEMP. Which usually results in a Access denied error.

It also ended up causing my PATH to not be completely evaluated so things like code wouldn't run from the command line.

I guess with enough entries it overflows some max string limit in Windows and there is no error dialog or indicator anywhere when this happens.

tmat · 2020-02-16T19:53:35Z

@Glen-Nicol-Garmin I have pushed a commit that removes Designer.cs file. We use a custom target to generate C# source from resx file that does not need a checked-in file.

tmat · 2020-02-16T20:10:43Z

src/SourceLink.GitWeb.UnitTests/GetSourceLinkUrlTests.cs

+                {
+                    // NOTE: i don't know what the spec parameter is for. but for all the other
+                    // tests like this for various providers it is the domain of the Repo URL.
+                    new MockItem("src.intranet.company.com", KVP("ContentUrl", "https://src.intranet.company.com/gitweb" + s2)),


new MockItem("src.intranet.company.com" [](start = 20, length = 39)

It is the host domain that needs to match the domain in the SourceRoot.

tmat

tmat · 2020-02-16T20:42:21Z

@Glen-Nicol-Garmin Thanks for the contribution!

Glen-Nicol-Garmin · 2020-02-17T16:46:10Z

Huzzah! That is great news! Before I forget, should I create an issue for that PATH bug in tests I mentioned?

tmat · 2020-02-17T18:36:55Z

@Glen-Nicol-Garmin No need. This should take care of it: #578.

tmat · 2020-02-17T19:00:30Z

@Glen-Nicol-Garmin The package is now available in the Azure feed:
Microsoft.SourceLink.GitWeb 1.1.0-beta-20117-01

Let me know if it works in a real-world project. Once you confirm I'll add a mention to Readme.md

Glen-Nicol-Garmin · 2020-02-18T21:15:33Z

Just tested it out. It works!

dfev77 · 2021-11-29T14:02:06Z

src/Common/TranslateRepositoryUrlGitTask.cs

+            {
+                Log.LogError(e.Message);
+                return;
+            }


Is it ok to just log and return? I'm thinking that the error might be observer only a few week/months later when one debugs the code and sees that source linking doesn't work anymore.
Why not let the exception propagate?

I am fairly hazy on the specifics now, but I think logging the error results in an MsBuild error that can't be suppressed. Is the concern that since it is a post build action that error may not be noticed since the files will be on disk? I think the exception being thrown would have the same limitation.

I don't remember the specific use case where the NotSupportedException was thrown to say whether or not the message is helpful enough to tell the user that their URL scheme/format is not supported by Sourcelink. But I thought those were the errors being thrown by the individual translators when they didn't support the repo url.