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

[ArPow] Onboard linker to arcade-powered source-build #2043

Closed
crummel opened this issue Feb 9, 2021 · 9 comments
Closed

[ArPow] Onboard linker to arcade-powered source-build #2043

crummel opened this issue Feb 9, 2021 · 9 comments
Assignees

Comments

@crummel
Copy link
Contributor

crummel commented Feb 9, 2021

No description provided.

@omajid
Copy link
Member

omajid commented Feb 10, 2021

PR: dotnet/linker#1823

@omajid
Copy link
Member

omajid commented Feb 24, 2021

PR was merged; can someone please look over the internal build to make sure things are okay?

@MichaelSimons
Copy link
Member

MichaelSimons commented Feb 25, 2021

I took a look at the internal build and noted the following:

  1. There are a number of System.* prebuilts that look like they would get resolved by adding a Version.Details.xml reference to SBRP.
    <Dependency Name="Microsoft.SourceBuild.Intermediate.source-build-reference-packages" Version="5.0.0-alpha.1.20473.1">
      <Uri>https://github.com/dotnet/source-build-reference-packages</Uri>
      <Sha>def2e2c6dc5064319250e2868a041a3dc07f9579</Sha>
      <SourceBuild RepoName="source-build-reference-packages" ManagedOnly="true" />
    </Dependency>

Update: Per #2085, we do not want to reference source-build-reference-packages

  1. I noticed in the resulting intermediate NuGet package that three packages are getting produced from the build. In 5.0, there is only one. I confirmed with @dseefeld, that he only sees one in his non-ArPow 6.0 changes as well. Functionally this by itself is not an issue. The concern is this may be a source of additional prebuilts that we haven't encountered before.

5.0 artifacts

  • Microsoft.NET.ILLink.Tasks

6.0 ArPow artifacts

  • Microsoft.NET.ILLink
  • Microsoft.NET.ILLink.Analyzers
  • Microsoft.NET.ILLink.Tasks

@MichaelSimons MichaelSimons added this to Stage 2 - CI implemented in Source-Build Project Feb 26, 2021
@omajid
Copy link
Member

omajid commented Mar 15, 2021

There's something strange about Microsoft.NET.ILLink.Analyzers.

$ ag Microsoft.NET.ILLink.Analyzers
src/ILLink.RoslynAnalyzer/ILLink.RoslynAnalyzer.csproj
14:    <PackageId>Microsoft.NET.ILLink.Analyzers</PackageId>

So the ILLink.RoslynAnalyzer project (which produces ILLink.RoslynAnalyzer.dll) produces the nuget package Microsoft.NET.ILLink.Analyzers.

ILLink.RoslynAnalzyer.dll itself is shipped with the 6.0 Preview 2 SDK:

$ tar tf ~/downloads/dotnet-sdk-6.0.100-preview.2.21155.3-linux-x64.tar.gz | grep -E -i 'illink[^/]*\.dll' | grep -v resources
./sdk/6.0.100-preview.2.21155.3/Sdks/Microsoft.NET.Sdk/analyzers/ILLink.RoslynAnalyzer.dll
./sdk/6.0.100-preview.2.21155.3/Sdks/Microsoft.NET.ILLink.Tasks/tools/net5.0/illink.dll
./sdk/6.0.100-preview.2.21155.3/Sdks/Microsoft.NET.ILLink.Tasks/tools/net5.0/ILLink.Tasks.dll
./sdk/6.0.100-preview.2.21155.3/Sdks/Microsoft.NET.ILLink.Tasks/tools/net472/ILLink.Tasks.dll

The Microsoft.NET.ILLink.Tasks package does not include this rolsyn dll:

$ unzip -l artifacts/packages/Release/Shipping/Microsoft.NET.ILLink.Tasks.6.0.100-dev.nupkg | grep -i roslyn

It seems like we would break the SDK build if we were to remove this and disallow prebuilts.

@omajid
Copy link
Member

omajid commented Mar 16, 2021

Comparing with 5.0 sdk, the 5.0 SDK doesn't include illink.roslynanalyzers:

$ tar tf dotnet-microsoft-built-sdk-5.0.104.tar.gz | grep -i link
./sdk/5.0.104/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.ILLink.targets
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/netcoreapp3.0/
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/netcoreapp3.0/ILLink.Tasks.dll
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/netcoreapp3.0/ILLink.Tasks.deps.json
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/netcoreapp3.0/illink.runtimeconfig.json
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/netcoreapp3.0/illink.dll
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/netcoreapp3.0/Mono.Cecil.dll
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/netcoreapp3.0/illink.deps.json
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/netcoreapp3.0/Mono.Cecil.Pdb.dll
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/net472/
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/net472/ILLink.Tasks.dll
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/net472/System.Reflection.Metadata.dll
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/net472/Mono.Cecil.dll
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/tools/net472/System.Collections.Immutable.dll
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/Icon.png
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/Sdk/
./sdk/5.0.104/Sdks/Microsoft.NET.ILLink.Tasks/Sdk/Sdk.props
$ tar tf dotnet-microsoft-built-sdk-5.0.104.tar.gz | grep -i illink | grep -i roslyn

The analyzer was a recent addition, post 5.0: dotnet/linker#1563

@MichaelSimons
Copy link
Member

Thanks @omajid, That explains the Microsoft.NET.ILLink.Analyzers package. What about the Microsoft.NET.ILLink package? I'm not seeing an SDK reference?

@omajid
Copy link
Member

omajid commented Mar 17, 2021

That one just contains illink.dll and resources. The dll here should already be included in the Microsoft.NET.ILLink package; could it really be bringing in additional dependencies?

unzip -l ./artifacts/source-build/self/src/artifacts/packages/Release/Shipping/Microsoft.NET.ILLink.6.0.100-dev.nupkg | awk '{ print $4 }'

Name
----
_rels/.rels
Microsoft.NET.ILLink.nuspec
lib/net5.0/cs/illink.resources.dll
lib/net5.0/de/illink.resources.dll
lib/net5.0/es/illink.resources.dll
lib/net5.0/fr/illink.resources.dll
lib/net5.0/it/illink.resources.dll
lib/net5.0/ja/illink.resources.dll
lib/net5.0/ko/illink.resources.dll
lib/net5.0/pl/illink.resources.dll
lib/net5.0/pt-BR/illink.resources.dll
lib/net5.0/ru/illink.resources.dll
lib/net5.0/tr/illink.resources.dll
lib/net5.0/zh-Hans/illink.resources.dll
lib/net5.0/zh-Hant/illink.resources.dll
lib/net5.0/illink.runtimeconfig.json
lib/net5.0/illink.dll
ref/net5.0/illink.dll
Icon.png
[Content_Types].xml
package/services/metadata/core-properties/389e43d829f24d9591523fc462093b43.psmdcp

Do you think it's worth removing it?

@MichaelSimons
Copy link
Member

Sorry, I should have cracked it open. It is curious though why this was excluded in past releases of source-build. If it is discovered to present a problem down the road, we can address it them. It would be better to avoid source-build "specializations" if not needed. Closing.

@omajid
Copy link
Member

omajid commented Mar 18, 2021

Thanks for making me double check things!

@MichaelSimons MichaelSimons moved this from Rollout to Completed in Source-Build Project May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

3 participants