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

Enable source-build through arcade #1823

Merged

Conversation

omajid
Copy link
Member

@omajid omajid commented Feb 10, 2021

This enables 'source-build', which makes it easier to build the entire shipping .NET SDK from source.

This is the first and second step of arcade-powered-source-build: https://github.com/dotnet/source-build/blob/master/Documentation/planning/arcade-powered-source-build/README.md

See dotnet/sourcelink#692 for a similar PR, that this is based on.

The LICENSE to LICENSE.txt rename is hack to work around NuGet/Home#7601 for now.

@omajid
Copy link
Member Author

omajid commented Feb 10, 2021

cc @crummel @dagood @dseefeld

@dagood
Copy link
Member

dagood commented Feb 10, 2021

error : The referenced project '../../../external/cecil/Mono.Cecil.csproj' does not exist. [/__w/1/s/src/linker/ref/Mono.Linker.csproj]

It looks like the submodule wasn't initialized by the Checkout step. The arcade-powered source-build CI template doesn't automatically provide submodule checkout yet: dotnet/source-build#2049. There's a workaround in there that I used for dotnet/source-build that can be copied here to start with, and getting rid of the workaround can be tracked there.

@@ -0,0 +1,148 @@
From 194c172bb1d8e0c0fa0596062ccdd3c28a3923b4 Mon Sep 17 00:00:00 2001
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are ways how to do nicely. Could you do it without patching sln?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any suggestions how? Looking at build.sh, it directly kicks off illnk.sln. I dont see too many options for conditionalization. Are you thinking of adding a separate sln file that we can use in a source-build context?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could add a build.proj file to do the actual build instead of building the .sln (and keep the .sln just for the IDE).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what Alexander said or add another configuration (e.g. Debug, Release, SourceBuild)to sln which does not include tests

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, I think.

@@ -0,0 +1,73 @@
From e2f6d8d0ff79a23433d205b9eb0c7526949bbe6b Mon Sep 17 00:00:00 2001
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This analyzer ships with SDK don't we want to build it as well?

Copy link
Member Author

@omajid omajid Feb 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm...it ships with the SDK? Can you point me to an SDK that includes it? I am looking at 5.0.103 and I can't find the analzyer:

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

If it ships with the SDK, we would definitely want to build it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a new in .NET6

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I am not sure how to handle this. Any ideas @crummel, @dagood @dseefeld?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should merge this PR as-is, and fix up the patches and this functional issue as followup. This PR represents the state of dotnet/source-build, it's not necessary to solve every source-build issue right here and now.

FYI @dseefeld this will also need to be fixed up in the old-infra-style .NET 6 preview1 work, once we find a resolution.

@omajid omajid force-pushed the arcade-powered-source-build-local-and-ci branch from 7864fe2 to d44a5ec Compare February 10, 2021 20:01
eng/Version.Details.xml Outdated Show resolved Hide resolved
@@ -84,6 +86,21 @@ stages:
${{ if eq(variables.officialBuild, 'true') }}:
displayName: Build and publish illink.sln $(_BuildConfig)

- job: SourceBuild_Managed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should disable tests processing and publishing if they are not built

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I am lost. Can you please help me understand how to do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now, the flag comes in through here:

https://github.com/mono/linker/blob/d1bb874351fd4652edd83285f4488739ea9fda7d/eng/azure-pipelines.yml#L36-L43

and get passed thorough the jobs template into the job template here:

https://github.com/mono/linker/blob/d1bb874351fd4652edd83285f4488739ea9fda7d/eng/common/templates/jobs/jobs.yml#L40-L52

Maybe you can add a enablePublishTestResults: false to the source-build job and it would override the "outer scope" enablePublishTestResults: true parameter. But AzDO might just throw an error if it's defined twice.

A more reliable solution might be to completely remove the "outer scope" enablePublishBuildArtifacts: true, and put it on each job that should publish test results.

@omajid omajid force-pushed the arcade-powered-source-build-local-and-ci branch 4 times, most recently from 7617c69 to a671d58 Compare February 16, 2021 16:32
This enables 'source-build', which makes it easier to build the entire
shipping .NET SDK from source.

This is the first and second step of arcade-powered-source-build:
https://github.com/dotnet/source-build/blob/master/Documentation/planning/arcade-powered-source-build/README.md

See dotnet/sourcelink#692 for a similar PR, that this is based on.

The `LICENSE` to `LICENSE.txt` rename is hack to work around
NuGet/Home#7601 for now.
@omajid omajid force-pushed the arcade-powered-source-build-local-and-ci branch from a671d58 to d19d9ed Compare February 16, 2021 16:35
@marek-safar marek-safar changed the title WIP: Enable source-build through arcade Enable source-build through arcade Feb 24, 2021
@marek-safar marek-safar merged commit 6b3a305 into dotnet:main Feb 24, 2021
agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022
Co-authored-by: Marek Safar <marek.safar@gmail.com>

Commit migrated from dotnet/linker@6b3a305
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

Successfully merging this pull request may close these issues.

None yet

4 participants