Skip to content
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

Feature: GitWeb Source Link provider #505

Merged
merged 3 commits into from Feb 16, 2020
Merged

Conversation

@Glen-Nicol-Garmin
Copy link
Contributor

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Nov 25, 2019

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
Copy link

@dnfclas dnfclas commented Nov 25, 2019

CLA assistant check
All CLA requirements met.

@tmat
Copy link
Member

@tmat tmat commented Nov 25, 2019

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
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Nov 26, 2019

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

@tmat
Copy link
Member

@tmat tmat commented Nov 26, 2019

A couple of days :)

Copy link
Member

@tmat tmat left a comment

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

Glen-Nicol-Garmin added a commit to Glen-Nicol-Garmin/sourcelink that referenced this issue Dec 2, 2019
@Glen-Nicol-Garmin
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Dec 2, 2019

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
Copy link
Member

@tmat tmat commented Dec 27, 2019

@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
Copy link
Member

@tmat tmat commented Dec 27, 2019

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
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Dec 30, 2019

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
Copy link
Member

@tmat tmat commented Jan 1, 2020

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
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Jan 2, 2020

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
Copy link
Member

@tmat tmat commented Jan 2, 2020

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
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Jan 2, 2020

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
Copy link
Member

@tmat tmat commented Jan 2, 2020

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
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Jan 2, 2020

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
Copy link
Member

@tmat tmat commented Jan 2, 2020

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
Copy link
Member

@tmat tmat commented Jan 3, 2020

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
tmat approved these changes Jan 3, 2020
@tmat
Copy link
Member

@tmat tmat commented Jan 3, 2020

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

@Glen-Nicol-Garmin
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Jan 3, 2020

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
Copy link
Member

@tmat tmat commented Jan 3, 2020

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
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Jan 6, 2020

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
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Jan 8, 2020

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

@Glen-Nicol-Garmin
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Jan 30, 2020

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
Copy link
Member

@tmat tmat commented Jan 30, 2020

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
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Jan 30, 2020

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

@tmat
Copy link
Member

@tmat tmat commented Jan 31, 2020

@Glen-Nicol-Garmin Yes, please rebase.

Glen-Nicol-Garmin added a commit to Glen-Nicol-Garmin/sourcelink that referenced this issue Jan 31, 2020
GitWeb only currently supports SSH repository URLs.

dotnet#505
@Glen-Nicol-Garmin
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Jan 31, 2020

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

Copy link
Member

@tmat tmat left a comment

🕐

GitWeb only currently supports SSH repository URLs.

dotnet#505
@Glen-Nicol-Garmin
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Feb 6, 2020

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
Copy link
Member

@tmat tmat commented Feb 16, 2020

@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.

{
// 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)),
Copy link
Member

@tmat tmat Feb 16, 2020

Choose a reason for hiding this comment

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

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 approved these changes Feb 16, 2020
Copy link
Member

@tmat tmat left a comment

:shipit:

@tmat tmat merged commit bdc85b7 into dotnet:master Feb 16, 2020
9 checks passed
@tmat
Copy link
Member

@tmat tmat commented Feb 16, 2020

@Glen-Nicol-Garmin Thanks for the contribution!

@Glen-Nicol-Garmin
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Feb 17, 2020

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

@tmat
Copy link
Member

@tmat tmat commented Feb 17, 2020

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

@tmat
Copy link
Member

@tmat tmat commented Feb 17, 2020

@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
Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin commented Feb 18, 2020

Just tested it out. It works!

{
Log.LogError(e.Message);
return;
}
Copy link

@dfev77 dfev77 Nov 29, 2021

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@Glen-Nicol-Garmin Glen-Nicol-Garmin Nov 29, 2021

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants