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

pdb files are not deterministic #9379

Closed
ctaggart opened this issue Jun 3, 2020 · 17 comments
Closed

pdb files are not deterministic #9379

ctaggart opened this issue Jun 3, 2020 · 17 comments

Comments

@ctaggart
Copy link
Contributor

ctaggart commented Jun 3, 2020

The goal Azure/AAD.fs#1 is to enable best practices by enabling Source Link and creating deterministic nuget packages. To have deterministic nuget packages, the assemblies & the pdb files must be deterministic. Following @clairernovotny's instructions, but it is not working for fsharp projects Azure/AAD.fs#3. The MSBuild log shows fsc is being passed --deterministic+, but the pdb file looks like:

image
(NuGet Package Explorer is just checking for a source path like /_/)

{"documents":{"C:\\Users\\cataggar\\io\\AAD.fs\\*":"https://raw.githubusercontent.com/Azure/AAD.fs/c40d415bc0358ff439e7e207f6a6d2640b3727e9/*"}}

It should looks like:

{"documents":{"/_/*":"https://raw.githubusercontent.com/Azure/AAD.fs/c40d415bc0358ff439e7e207f6a6d2640b3727e9/*"}}

Steps to reproduce:

git clone git@github.com:Azure/AAD.fs.git
git checkout deterministic
dotnet build AAD.fs /t:clean,build /p:TF_BUILD=true /v:n
dotnet tool install --global sourcelink
sourcelink print-json AAD.fs\bin\Debug\netstandard2.0\AAD.fs.pdb

Windows 10 Version 2004
dotnet --version
3.1.300

This looks like relevant Roslyn code: cc @tmat
https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/MSBuildTask/MapSourceRoots.cs#L14-L26

@clairernovotny
Copy link
Member

The /_/ part comes from ContinousIntegrationBuild set to true, which should only be done on CI builds since local debugging won't work with that set.

@cartermp
Copy link
Contributor

cartermp commented Jun 3, 2020

What happens if you use PathMap in the project file? Perhaps the F# targets just need to get updated to do that by default, since I'm pretty sure they don't

<PathMap>C:\doot=/</PathMap>

@clairernovotny
Copy link
Member

I'm not following?

@ctaggart
Copy link
Contributor Author

ctaggart commented Jun 3, 2020

@clairernovotny, yes, ContinousIntegrationBuild is being set. That makes it so --deterministic+ is passed to fsc. Based on the docs at https://github.com/dotnet/sourcelink/tree/master/docs#deterministicsourcepaths, I've also tried hardcoding all of these options without luck:

    <ContinuousIntegrationBuild>true</ContinuousIntegrationBuild>
    <Deterministic>true</Deterministic>
    <DeterministicSourcePaths>true</DeterministicSourcePaths>

@clairernovotny
Copy link
Member

@tmat?

@ctaggart
Copy link
Contributor Author

ctaggart commented Jun 3, 2020

@cartermp, thanks for the PathMap suggestion. I tried various options, but none worked. I tried each of these:

<PathMap>C:\Users\cataggar\io\AAD.fs\=/</PathMap>
<PathMap>C:\Users\cataggar\io\AAD.fs\=/</PathMap>
<PathMap>C:\Users\cataggar\io\AAD.fs\*=/</PathMap>
<PathMap>C:\\Users\\cataggar\\io\\AAD.fs\\=/</PathMap>
<PathMap>C:/Users/cataggar/io/AAD.fs/=/</PathMap>
<PathMap>C:\Users\=/</PathMap>

And I could see each option being passed to fsc, such as --pathmap:C:\Users\=/ for the lat one. fsc says that it is a valid option:

. "C:\Program Files\dotnet\dotnet.exe" "C:\Program Files\dotnet\sdk\3.1.300\FSharp\fsc.exe" --help

--pathmap:<path=sourcePath;...>          Maps physical paths to source path names output by the compiler

It just does not have an impact on the produced sourcelink json.

@tmat
Copy link
Member

tmat commented Jun 3, 2020

F# compiler msbuild targets might be missing SourceRoot mapping logic:
https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/MSBuildTask/Microsoft.Managed.Core.targets#L143-L227

Besides that we will need F# to implement this feature as well at some point (for next level badge):
dotnet/roslyn#41395

@ctaggart
Copy link
Contributor Author

ctaggart commented Jun 3, 2020

Using the example project, I see the SourceRoot mapping logic which is in this file on my laptop:
"C:\Program Files\dotnet\sdk\3.1.300\Roslyn\Microsoft.Managed.Core.targets"
Results in this csc option being passed in:

/pathmap:"C:\Users\cataggar\.nuget\packages\=/_1/,C:\Users\cataggar\io\DeterministicBuilds\=/_/"

Based on that, I hardcoded this:

<PathMap>C:\Users\cataggar\.nuget\packages\=/_1/;C:\Users\cataggar\io\AAD.fs\=/_/</PathMap>

Which made it to fsc as option:

--pathmap:C:\Users\cataggar\.nuget\packages\=/_1/,C:\Users\cataggar\io\AAD.fs\=/_/

but no impact on the produced sourcelink json. 😢

@tmat
Copy link
Member

tmat commented Jun 3, 2020

That's because PathMap is csc parameter. The SourceRoot items themselves need to be updated like so in order to be picked up by Source Link.

@tmat
Copy link
Member

tmat commented Jun 3, 2020

Perhaps we should move these targets down to the msbuild Common targets, so that all languages can benefit from a common path mapping implementaiton.

@clairernovotny
Copy link
Member

@tmat I agree as it's important that this functionality is available everywhere. Do you want to create the work item in Roslyn for it or should I?

@ctaggart
Copy link
Contributor Author

ctaggart commented Jun 3, 2020

That's because PathMap is csc parameter. The SourceRoot items themselves need to be updated like so in order to be picked up by Source Link.

Oh, got it. I see SourceRoot is what Source Link uses. It is just passing --pathmap to the compilers based on it. I'm currently wondering if there is a way to include Roslyn\Microsoft.Managed.Core.targets and run the required targets with the current sdk.

@jackfoxy
Copy link

jackfoxy commented Jun 3, 2020

Last time I tried, I could not debug into source-linked F# DLLs in VS. Getting source linked debugging working is high on my priority list. I searched and there is no open issue for this specifically.

@tmat
Copy link
Member

tmat commented Jun 3, 2020

@clairernovotny The work would be in msbuild Common targets, not in Roslyn.

@ctaggart I don't think you should include Roslyn\Microsoft.Managed.Core.targets in F# targets as it has logic that is Roslyn-specific. You could copy the targets and task that perform the source path mapping to F# targets.

@baronfel
Copy link
Member

baronfel commented Aug 1, 2020

This is possibly a duplicate of #8883, what do you think? Also, is there any chance of the targets getting moved in time for .Net 5/F# 5.0? If not, does F# want to embed the task/targets to enable this before that time?

@cartermp
Copy link
Contributor

cartermp commented Aug 2, 2020

Yes, good catch - this should be closed in favor of #8883

I think we should strive to re-use this in F# even if it means a little copypasta.

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

No branches or pull requests

6 participants