-
Notifications
You must be signed in to change notification settings - Fork 36
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
Enable DeterministicSourcePaths. #281
Conversation
Actually, it did not regress there. Each project had |
Is this intended to completely fix dotnet/source-build#4011 or just contribute to fixing it? Because the original post of that issue is referring to System.Collections.dll which would be unaffected by these changes. |
That was the intention, but indeed it does not fix all issues. runtime seems to be producing both dlls with and without the pathmapping. I'll take a look at what is going on there. |
I took a look and wrote up an issue in runtime repo with some findings: dotnet/runtime#99594. |
Based on my builds, this has the desired effect of masking the build server vmr path. This should be good to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think these changes are necessary. This configuration is already done at the root level: https://github.com/dotnet/dotnet/blob/8c1aeea68e7274f210d9eb210f2451444013d214/repo-projects/Directory.Build.props#L147-L148
This flows into the build of source-build-externals and, from there, into the builds of the submodules it contains. I've verified this in the binlog.
As a minimum, we need to remove the If we don't need to add an |
I guess the difference would be in whether the variables are necessary for a standalone build of source-build-externals (not in the context of the VMR). Do you see it being necessary to support deterministic paths for a standalone build of source-build-externals? If so, then your original changes are good. |
FWIW @MichaelSimons mentioned the following in #115 which changed the default:
|
I only care about when this gets built as part of the VMR. I would assume this is true for the other envvars set in the file as well. I'll remove the DeterministicSourcePaths envvars. |
Why would we want to enable deterministic source paths only inside the VMR? What's the technical reason for that? I would assume that we want to keep the diff between the repo and the VMR build as small as possible. |
I also prefer to keep the build difference minimal. I understood @mthalman's feedback to mean he would prefer to leave the lines out, so I've applied that change. I'll revert it. |
This commit introduced the opt-out switch: 4303e22 I noticed that it was initially only set for a few projects and only two of those still exist: Please test that with your change those two projects still have the correct sourcelink information set (in addition to the deterministic source paths). My guess is that the opt-out switch isn't necessary anymore as the task that got added with the above commit which adds sourcelink metadata, doesn't exist anymore: https://github.com/search?q=repo%3Adotnet%2Fdotnet%20WriteSourceRepoProperties&type=code Unfortunately the commit message doesn't provide much information and I can't find the underlying PR. @MichaelSimons would you know? |
Here's the original PR: dotnet/source-build#1168. Unfortunately there's not a description. |
This predates my time on source-build. Your assessment seems logical to me. |
Why do these two projects need special handling? I assume that without special handling, https://github.com/dotnet/dotnet is the source root for the whole build, which would be the simplest option. |
Contributes to dotnet/source-build#4011.
@MichaelSimons @ViktorHofer ptal.
cc @omajid