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

DeterministicSourcePaths can break building if source control information not available #37379

Closed
KevinCathcart opened this issue Jul 19, 2019 · 5 comments
Assignees
Labels
Area-Compilers Resolution-Answered The question has been answered Resolution-By Design The behavior reported in the issue matches the current design
Milestone

Comments

@KevinCathcart
Copy link

Version Used:
dotnet sdk 2.2.300
Steps to Reproduce:

  1. dotnet new console
  2. dotnet build /p:ContinuousIntegrationBuild=true

Actual Behavior:

C:\Program Files\dotnet\sdk\2.2.300\Roslyn\Microsoft.Managed.Core.targets(102,5): error : SourceRoot items must include at least one top-level (not nested) item when DeterministicSourcePaths is true

Expected Behavior:
Build Succeeds.

The mere fact that we we are performing a continuous integration build does not mean that source control information will be available to MSBuild. However deterministic builds are enabled by default, and turning on ContinuousIntegrationBuild, means that DeterministicSourcePaths is enabled.

That would be fine if DeterministicSourcePaths worked when no source control information was available, but it requires SourceRoot items, which are only created by a source control information provider like the Source Link packages.

This bit me when I tried to add a new Test project to a solution being built with /p:ContinuousIntegrationBuild=true. I did not bother to add a Source Link NuGet package since I was not going to package or publish the test project on the CI server, merely run it, so I don't actually need Source Link data in the pdbs.

Obviously I know the workarounds (add the Source Link package or set DeterministicSourcePaths to false in the test project).

But ideally they would not be needed. Instead ideally, either a) DeterministicSourcePaths would only be set if a source control information provider package was installed, or b) that DeterministicSourcePaths was somehow made to work even without a source provider package.

@tmat I think you are the expert on this stuff, so this is probably your issue.

@tmat
Copy link
Member

tmat commented Jul 19, 2019

DeterministicSourcePaths does not need source control information. It just needs to know where the repository root is located.

You can define the repository root by adding the following to the Directory.Build.props file in the root dir:

<ItemGroup>
  <SourceRoot Include="$(MSBuildThisFileDirectory)/"/>
</ItemGroup>

@jcouv jcouv added the Resolution-Answered The question has been answered label Jul 19, 2019
@KevinCathcart
Copy link
Author

tmat, that is strictly speaking true, but other than manually adding a SourceRoot element, or using a Source Link package, there is no mechanism for populating that. I would also argue that calling any SourceRoot item source control information is proper, given that the comments already call it that. (Any target that reads [...], SourceRoot, and other source control properties and items [...]).

My gripe is that with a property as generically named as "ContinuousIntegrationBuild" (which is likely to grow additional functionality over time, since it is the obvious mechanism for informing the SDK that we are running in a CI server), it really should not be necessary to make any source level changes.

My real preference would be to downgrade that error to a warning. (Perhaps only if the developer did not explicitly set DeterministicSourcePaths?) Another reasonable alternative would be to introduce some special MSBuild property that if set would generate a SourceRoot item. It would be easy to configure the CI server set that property to the root of the "workspace" (or whatever it happens to call the equivalent concept).

@tmat
Copy link
Member

tmat commented Jul 24, 2019

@KevinCathcart The problem is that msbuild does not know what the repository root directory is - it does not have a concept of repository. Someone has to determine that. SourceLink is able to do that based on the source control information. As I pointed out above another option is to specify it explicitly in Directory.Build.props. Yet another option is to read CI environment variable as you suggest. The problem is that there is no standard naming of these variables, so far we have a proposal for such naming but it's not being implemented yet.

The intention is that ContinuousIntegrationBuild is going to be used for builds on CI server and that these builds are by default configured to be fully deterministic, even if that means requiring additional settings and reporting an error if you don't have them in place (or if your projects don't use some package like SourceLink that fills in these settings).

@tmat tmat added the Resolution-By Design The behavior reported in the issue matches the current design label Jul 24, 2019
@gafter gafter added this to the 16.3 milestone Jul 25, 2019
@gafter
Copy link
Member

gafter commented Jul 25, 2019

@jaredpar Please close if you think that is appropriate.

@KevinCathcart
Copy link
Author

@tmat:

The intention is that ContinuousIntegrationBuild is going to be used for builds on CI server and that these builds are by default configured to be fully deterministic, even if that means requiring additional settings

One problem is that you don't offer these additional settings in a clean way. The only options involve adding or modifying files in the source repository. That is a bad thing. If the build server needs to supply additional information, like the root of the checked out repo, it should be able to do that via MSBuild properties.

Otherwise, it means either: one cannot build older revisions deterministically, or you can by having the build server drop in extra files, or modify some file checked into source control. Anybody who cares about deterministic builds would NOT want the build server adding files to the build, or modifying the files in the build.

@jaredpar jaredpar closed this as completed Sep 9, 2019
TimothyMothra pushed a commit to microsoft/ApplicationInsights-dotnet that referenced this issue May 7, 2020
adding debugtype as portable to check symbols

removing os condition

update changelog for removing netstandard 1.x (#1858)

since we are using msbuild, we have to set deterministic

reverting

updating Product.props and removing reference from all projects

adding sourceroot based on github issue (dotnet/roslyn#37379)

including pdbs in output folder

commenting deterministic properties and sourceroot

removing cibuild from nugetprops

undoing nupkg and sourceroot from directory.build
ascott18 added a commit to IntelliTect/Coalesce that referenced this issue Jul 14, 2020
laedit added a commit to laedit/vika that referenced this issue Oct 13, 2021
Adding SourceRoot which is needed for ContinuousIntegrationBuild and Deterministic: dotnet/roslyn#37379
Otherwise wa have this error:
Microsoft.Managed.Core.targets(191,5): error : SourceRoot items must include at least one top-level (not nested) item when DeterministicSourcePaths is true
kekyo added a commit to kekyo/RelaxVersioner that referenced this issue Mar 23, 2022
reduckted added a commit to reduckted/Community.VisualStudio.Toolkit that referenced this issue Apr 22, 2022
nachtjasmin added a commit to anexia/dotnet-e5e that referenced this issue Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Resolution-Answered The question has been answered Resolution-By Design The behavior reported in the issue matches the current design
Projects
None yet
Development

No branches or pull requests

6 participants