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

Some build targets run when ExcludeFromSourceBuild is set #7795

Open
2 tasks
dseefeld opened this issue Aug 25, 2021 · 8 comments
Open
2 tasks

Some build targets run when ExcludeFromSourceBuild is set #7795

dseefeld opened this issue Aug 25, 2021 · 8 comments

Comments

@dseefeld
Copy link
Contributor

  • This issue is blocking
  • This issue is causing unreasonable pain

In some cases, when a project is marked with ExcludeFromSourceBuild=true, the build is still running some targets that may lead to unexpected consequences. This was discovered by RedHat when building using a new RID.

Even though a project was marked with ExcludeFromSourceBuild, it was still calling the ProcessFrameworkReferences target which ended up calling the ResolveAppHosts which failed on the new RID. But, the assumption is that if the project was marked ExcludeFromSourceBuild, it should not be doing anything with this project.

I reproed this issue locally by building arcade with source-build ./build.sh /p:ArcadeBuildFromSource=true and looking at the binlog at ./artifacts/source-build/self/src/artifacts/sourcebuild.binlog. There are 3 projects to consider in the output:

  • Microsoft.DotNet.Arcade.Sdk.csproj - Has ExcludeFromSourcebuild set to false & builds normally.
  • Microsoft.DotNet.Build.Tasks.Feed.csproj - Has ExcludeFromSourceBuild set to true and doesn't execute any targets.
  • Microsoft.DotNet.GitSync.CommitManager.csproj - Has ExcludeFromSourcebuild set to true and executes some targets.

The difference between the 2nd and 3rd projects is that Microsoft.DotNet.Build.Tasks.Feed.csproj specifies multiple TargetFrameworks while Microsoft.DotNet.GitSync.CommitManager.csproj specifies a single TargetFramework. Because of this difference, the last project includes Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.BeforeCommon.targets instead of Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.BeforeCommonCrossTargeting.targets. That in turn imports Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.RuntimeIdentifierInference.targets which seems to include the targets that are being run.

As a workaround, I added the following targets to https://github.com/dotnet/arcade/blob/main/src/Microsoft.DotNet.Arcade.Sdk/tools/Empty.targets which eliminated calling the targets:
<Target Name="ApplyImplicitVersions"/>
<Target Name="_CollectTargetFrameworkForTelemetry"/>
<Target Name="ProcessFrameworkReferences"/>
<Target Name="CollectPackageReferences"/>
<Target Name="_CheckForInvalidConfigurationAndPlatform"/>

@ghost ghost added the Needs Triage A new issue that needs to be associated with an epic. label Aug 25, 2021
@MattGal
Copy link
Member

MattGal commented Aug 26, 2021

[Async Triage] : Seems like it needs to happen, no idea where it goes epic-wise. With specific guidance, fixing this for just Arcade could be Expanded FR.

@ghost ghost removed the Needs Triage A new issue that needs to be associated with an epic. label Sep 10, 2021
@jakubstilec
Copy link
Contributor

[Async Triage]: @riarenas was it too big for FR, should we convert to small epic?

@riarenas
Copy link
Member

The fact that I moved it to "in PR" was a mistake, I was probably trying to move another issue, and @adiaaida fixed my mistake during a standup. I don't really have any context on this one.

I don't have a sense for how big this is, but given Matt's comment and the issue itself it seems bigger than a regular FR issue, and doesn't quite fit the bill since neither the blocking or unreasonable pain checkboxes were marked, and there seems to be a workaround in place.

I don't think it's big enough for its own small epic, but expanded FR work also seems like it rarely gets to be worked on. I'm not sure where we want to fit this.

@dseefeld
Copy link
Contributor Author

Also, see the comment here: dotnet/aspnetcore#35846 (comment) It seems this needs to be coupled with an SDK change.

@markwilkie
Copy link
Member

Is this something you want to see through and submit a PR for is appropriate @dseefeld ?

@dseefeld
Copy link
Contributor Author

The issue is still there. I had a PR to resolve it in Arcade (#7825) but closed it because of the comment linked above: dotnet/aspnetcore#35846 (comment). I'd like to see this fixed, either in the SDK or in arcade.

@markwilkie
Copy link
Member

Do you have a recommend @dseefeld on what we should do?

@dseefeld
Copy link
Contributor Author

If we're not going to do anything in the SDK, I'd like to see it fixed in Arcade. The PR I had closed will fix the issue. (#7825)

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

Successfully merging a pull request may close this issue.

6 participants