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.

Sign up for GitHub

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

Jump to bottom

add source-build pre-built detection #933

Merged
merged 11 commits into from May 16, 2023
Merged

add source-build pre-built detection #933

merged 11 commits into from May 16, 2023

Conversation

oleksandr-didyk
Copy link
Contributor

@oleksandr-didyk oleksandr-didyk commented Jan 2, 2023 •
edited

Resolves #932

Additionally created a subscription for the dependency on runtime (disabled until this PR is merged):

https://github.com/dotnet/runtime (.NET 8) ==> 'https://github.com/dotnet/sourcelink' ('main')
  - Id: e02204af-e240-492f-2682-08daecc8f206
  - Update Frequency: EveryWeek
  - Enabled: False
  - Batchable: False
  - PR Failure Notification tags:
  - Merge Policies: []

CC: @MichaelSimons @crummel

oleksandr-didyk
oleksandr-didyk commented Jan 2, 2023
View reviewed changes
azure-pipelines.yml Outdated Show resolved Hide resolved
@KalleOlaviNiemitalo
Copy link

KalleOlaviNiemitalo commented Jan 2, 2023 •
edited

Will the title of this PR be copied to the release notes as is? If so, please include "source-build" in it. I totally expected "pre-build detection" to be a new feature that detects if the CI pipeline has run nbgv get-version already and then skips the NBGV tasks during MSBuild.

Uh I suppose I got confused about the repository as well.

@oleksandr-didyk oleksandr-didyk changed the title add pre-build detection add source-build pre-build detection Jan 3, 2023
@oleksandr-didyk oleksandr-didyk changed the title add source-build pre-build detection add source-build pre-built detection Jan 3, 2023
@oleksandr-didyk oleksandr-didyk mentioned this pull request Jan 3, 2023
Closed
27 tasks
tmat
tmat reviewed Jan 9, 2023
View reviewed changes
eng/Versions.props Outdated Show resolved Hide resolved
MichaelSimons
MichaelSimons reviewed Jan 11, 2023
View reviewed changes
eng/cibuild.sh Outdated Show resolved Hide resolved
@oleksandr-didyk oleksandr-didyk mentioned this pull request Feb 1, 2023
Merged
@mthalman
Copy link
Member

@oleksandr-didyk - Can you give a status on this please? Is there in-progress work or is it blocked or just pending work? Thanks

@oleksandr-didyk
Copy link
Contributor Author

@oleksandr-didyk - Can you give a status on this please? Is there in-progress work or is it blocked or just pending work? Thanks

It was initially waiting for the Arcade bump to flow in. The change got into main last week so this PR got unblocked.
I updated it and it should be ready for re-review now

MichaelSimons
MichaelSimons reviewed Apr 11, 2023
View reviewed changes
eng/Version.Details.xml Show resolved Hide resolved
eng/SourceBuildPrebuiltBaseline.xml Outdated Show resolved Hide resolved
@MichaelSimons MichaelSimons mentioned this pull request Apr 11, 2023
Closed
tmat
tmat reviewed Apr 13, 2023
View reviewed changes
eng/Versions.props Outdated Show resolved Hide resolved
@ViktorHofer ViktorHofer mentioned this pull request Apr 15, 2023
Merged
ViktorHofer
ViktorHofer reviewed Apr 18, 2023
View reviewed changes
NuGet.config Outdated Show resolved Hide resolved
@ViktorHofer
Copy link
Member

OK there's one more PR that I just submitted that will again make things easier and this PRs diff much smaller: #1003. I suggest to also wait for that one.

@oleksandr-didyk oleksandr-didyk force-pushed the feat-enable-pre-build-detection branch 3 times, most recently from c9d7ba3 to 99d65a8 Compare May 4, 2023 14:38
@oleksandr-didyk oleksandr-didyk requested a review from tmat May 4, 2023 14:39
ViktorHofer
ViktorHofer reviewed May 4, 2023
View reviewed changes
src/dotnet-sourcelink/dotnet-sourcelink.csproj
@@ -4,6 +4,7 @@
<TargetFrameworks>$(NetCurrent)</TargetFrameworks>
<!-- Allow tool to roll forward to a newer major version. -->
<RollForward>Major</RollForward>
<ExcludeFromSourceBuild>true</ExcludeFromSourceBuild>
Copy link
Member

Choose a reason for hiding this comment

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

Why does this tool need to be excluded from source build? Please add a comment as it isn't clear just from reading the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it based on the project's description:

<Description>Command line tool for SourceLink testing.</Description>

Since I thought it is a tool used for testing, but reading it again I might be wrong here. Maybe @tmat can clarify it

Additionally

<PackAsToolShimRuntimeIdentifiers>win-x64;win-x86;osx-x64</PackAsToolShimRuntimeIdentifiers>

causes some additional packages to get pulled in that are marked as pre-builts:

    <Usage Id="Microsoft.AspNetCore.App.Runtime.osx-x64" Version="8.0.0-preview.3.23177.8" />
    <Usage Id="Microsoft.AspNetCore.App.Runtime.win-x64" Version="8.0.0-preview.3.23177.8" />
    <Usage Id="Microsoft.AspNetCore.App.Runtime.win-x86" Version="8.0.0-preview.3.23177.8" />
    <Usage Id="Microsoft.NETCore.App.Host.osx-x64" Version="8.0.0-preview.3.23174.8" />
    <Usage Id="Microsoft.NETCore.App.Host.win-x64" Version="8.0.0-preview.3.23174.8" />
    <Usage Id="Microsoft.NETCore.App.Host.win-x86" Version="8.0.0-preview.3.23174.8" />
    <Usage Id="Microsoft.NETCore.App.Runtime.osx-x64" Version="8.0.0-preview.3.23174.8" />
    <Usage Id="Microsoft.NETCore.App.Runtime.win-x64" Version="8.0.0-preview.3.23174.8" />
    <Usage Id="Microsoft.NETCore.App.Runtime.win-x86" Version="8.0.0-preview.3.23174.8" />

Copy link
Member

Choose a reason for hiding this comment

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

It should be included. This is not a shipping product yet but it is meant to be. It's for customers to test that their binaries have correct Source Link.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not a shipping product yet

Can this be done as a separate issue to not block this PR? Or it being excluded for some time is a no-go?
Specifically since the assets pulled in by PackAsToolShimRuntimeIdentifiers are non-Linux; or can this property be simply excluded? (as source-build is Linux-only for now)

Copy link
Member

Choose a reason for hiding this comment

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

IMO if source build repositories currently don't depend on the tool, it shouldn't be an issue to exclude it for now and track enabling it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue for it -> #1028

@oleksandr-didyk oleksandr-didyk force-pushed the feat-enable-pre-build-detection branch from d790a53 to 81716f5 Compare May 15, 2023 16:08
@tmat tmat merged commit d6c5a50 into dotnet:main May 16, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rainersigwald rainersigwald rainersigwald left review comments

@ViktorHofer ViktorHofer ViktorHofer left review comments

@tmat tmat tmat approved these changes

@MichaelSimons MichaelSimons Awaiting requested review from MichaelSimons

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable source-build pre-built detection
7 participants
@oleksandr-didyk @KalleOlaviNiemitalo @mthalman @ViktorHofer @tmat @rainersigwald @MichaelSimons