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.

Sign up for GitHub

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

Integrate GitLink into the Roslyn build #15592

Merged
merged 1 commit into from
Nov 30, 2016
Merged

Integrate GitLink into the Roslyn build #15592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build/Targets/Dependencies.props
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
<Project ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
<!--Generated file, do not directly edit. Run "RepoUtil change" to regenerate-->
<PropertyGroup>
<GitLinkVersion>2.3.0</GitLinkVersion>
<ManagedEsentVersion>1.9.4</ManagedEsentVersion>
<MicroBuildCoreVersion>0.2.0</MicroBuildCoreVersion>
<MicroBuildCoreSentinelVersion>1.0.0</MicroBuildCoreSentinelVersion>
Expand Down
4 changes: 3 additions & 1 deletion build/ToolsetPackages/project.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@
"RoslynDependencies.OptimizationData": "2.0.0-rc-61101-16",
"RoslynTools.Microsoft.LocateVS": "0.2.0-beta",
"RoslynTools.Microsoft.SignTool": "0.2.0-beta",
"RoslynTools.Microsoft.VSIXExpInstaller": "0.2.1-beta"
"RoslynTools.Microsoft.VSIXExpInstaller": "0.2.1-beta",
"GitLink": "2.3.0"

},
"frameworks": {
"net461": {}
Expand Down
3 changes: 2 additions & 1 deletion build/config/RepoUtilData.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@
"Microsoft.CodeAnalysis.Elfie",
"xunit",
"RoslynTools.*",
"^Microsoft.?Build.*"
"^Microsoft.?Build.*",
"GitLink"
]
}
},
Expand Down
2 changes: 2 additions & 0 deletions src/Tools/MicroBuild/Build.proj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@

<MSBuild Projects="$(ProjectDir)Roslyn.sln" BuildInParallel="true" Properties="DeployExtension=false" />

<Exec Command="$(NuGetPackageRoot)\GitLink\$(GitLinkVersion)\lib\net45\GitLink.exe $(ProjectDir) -u https://github.com/dotnet/roslyn -f Roslyn.sln -c $(Configuration)" />
Copy link
Member

Choose a reason for hiding this comment

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

Are we still tracking the "make this run for PRs once we fix the perf issues"? I wholeheartedly approve this and don't want to block, but still want to make sure we're tracking the improvement somewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's chat about this in person. The perf issue is no longer relevant with GitLink. But I'm not sure it's going to have the effect we need.

Copy link

Choose a reason for hiding this comment

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

PR's are going to be difficult, see GitTools/GitLink#124

Copy link

Choose a reason for hiding this comment

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

About performance: @AArnott is helping us working on a pdb-only version. This should no longer require the tool to load the solution making it much faster. This will be v3+.

Copy link
Member Author

Choose a reason for hiding this comment

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

@GeertvanHorrik can you clarify why you think PRs are going to be difficult here? Reading that bug it seems to be about finding the target url. This is specified explicitly here hence detection shouldn't happen.

The reason we want this enabled for PRs is crash dump investigation. Our tests are configured such that in certain bad cases it will archive out our EXE / PDB combo for future investigation. Much easier to farm that investigation out to the relevant developer (not necessarily guy who make the PR) if we have source server support in the PDB.

Copy link

Choose a reason for hiding this comment

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

@jaredpar It depends per source server, but in GitHub it's hard to find the actual raw files on the server (since it's a merge of both original repository + fork). I've been doing some testing, but couldn't find a reliable way to find the actual raw files for a PR.

If you have a reliable way to specify the url (because I don't think the specified url in this script will point to the right PR-files), I would love to hear about that.

I do understand why you want it for PR's (although it's a very rare use-case for most people), but it makes total sense.

Copy link
Member Author

@jaredpar jaredpar Dec 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@GeertvanHorrik GitLink should know the following two pieces of information:

  • GitHub repo URL
  • Commit SHA

Given that any file should be reachable at the following:

<github repo url>/blob/<git sha>/<relative path to file>

This logic doesn't need to take into account forks in the PR case. By virtue of creating the PR, the corresponding commit SHA will be available in the original repo (just verified to be sure).


<Exec Command="powershell -noprofile -executionPolicy ByPass -file $(MSBuildThisFileDirectory)..\..\..\build\scripts\check-nuspec.ps1 &quot;$(MSBuildThisFileDirectory)..\..\..&quot; &quot;$(BinariesPath)&quot;" />
<Exec Command="powershell -noprofile -executionPolicy ByPass -file $(MSBuildThisFileDirectory)..\..\..\build\scripts\create-perftests.ps1 &quot;$(BinariesPath)&quot;" />

Expand Down