Skip to content

add source-build pre-built detection #274

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
Merged
mmitche merged 6 commits into from
Apr 3, 2023
Merged

add source-build pre-built detection #274

mmitche merged 6 commits into from
Apr 3, 2023

Conversation

@oleksandr-didyk
Copy link
Contributor

@oleksandr-didyk oleksandr-didyk commented Jan 4, 2023

Resolves #273

Additionally created a disabled subscription for the added dependency on source-build reference packages (will enable once this PR is merged):

https://github.com/dotnet/source-build-reference-packages (.NET Eng - Latest) ==> 'https://github.com/dotnet/symreader' ('main')
  - Id: d02bc019-13df-492f-4dee-08daede62b8f
  - Update Frequency: EveryDay
  - Enabled: False
  - Batchable: False
  - PR Failure Notification tags:
  - Merge Policies:
    AllChecksSuccessful
      ignoreChecks =
                       [
                         "WIP",
                         "license/cla"
                       ]

CC: @MichaelSimons @crummel

@oleksandr-didyk oleksandr-didyk mentioned this pull request Jan 4, 2023
Closed
27 tasks
MichaelSimons
MichaelSimons reviewed Jan 4, 2023
View reviewed changes
eng/Versions.props Outdated
<UsingToolNetFrameworkReferenceAssemblies>true</UsingToolNetFrameworkReferenceAssemblies>
<UsingToolNuGetRepack>true</UsingToolNuGetRepack>
<!-- Dependencies -->
<MicrosoftSourceBuildIntermediatesourcebuildreferencepackagesPackageVersion>8.0.0-alpha.1.22616.1</MicrosoftSourceBuildIntermediatesourcebuildreferencepackagesPackageVersion>
Copy link
Member

@MichaelSimons MichaelSimons Jan 4, 2023

Choose a reason for hiding this comment

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

Curious how you created these? Is there tooling that will add these and the version details dependency entry or were these created by hand?

Copy link
Contributor Author

@oleksandr-didyk oleksandr-didyk Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Darc created these through darc add-dependency + updates the values once darc update-dependency is called (guess the same is done if the update comes from a PR)

Copy link
Member

@MichaelSimons MichaelSimons Jan 4, 2023

Choose a reason for hiding this comment

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

Thanks - I've always added these by hand. It's good to know.

Copy link
Member

@tmat tmat Jan 8, 2023

Choose a reason for hiding this comment

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

MicrosoftSourceBuildIntermediatesourcebuildreferencepackagesPackageVersion this is hard to read, can it be capitalized correctly?

Copy link
Contributor Author

@oleksandr-didyk oleksandr-didyk Jan 10, 2023

Choose a reason for hiding this comment

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

It is created / updated by tooling (darc), not sure it will work with a different definition

Copy link
Member

@mmitche mmitche Jan 11, 2023

Choose a reason for hiding this comment

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

I think it should work with any capitalization.

Copy link
Contributor Author

@oleksandr-didyk oleksandr-didyk Jan 12, 2023

Choose a reason for hiding this comment

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

Or maybe it can just be removed from the Versions.props completely?

Copy link
Member

@mmitche mmitche Apr 3, 2023

Choose a reason for hiding this comment

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

Yep, not needed in Versions.props

MichaelSimons
MichaelSimons reviewed Jan 4, 2023
View reviewed changes
eng/Version.Details.xml Outdated
<Sha>80b6be47e1425ea90c5febffac119250043a0c92</Sha>
<SourceBuild RepoName="arcade" ManagedOnly="true" />
</Dependency>
<Dependency Name="Microsoft.SourceBuild.Intermediate.source-build-reference-packages" Version="8.0.0-alpha.1.22616.1">
Copy link
Member

@MichaelSimons MichaelSimons Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We are inconsistent across the repos in regards to SBRP being a tooling versus product dependency. @mmitche, we've talked about this in the past. Now that you know more about source-build what should this be classified as? I think it should be a product dependency.

Copy link
Member

@mmitche mmitche Jan 6, 2023

Choose a reason for hiding this comment

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

I think it should be product too. It may not make a ton of difference, but product dependencies have more restrictions (can't have cycles, show up in coherency calculations in BARViz).

Copy link
Member

@tmat tmat Jan 8, 2023

Choose a reason for hiding this comment

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

Why is this package name using - for separators?

Copy link
Contributor Author

@oleksandr-didyk oleksandr-didyk Jan 10, 2023

Choose a reason for hiding this comment

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

Maybe @MichaelSimons could help answer that

Copy link
Member

@MichaelSimons MichaelSimons Jan 10, 2023

Choose a reason for hiding this comment

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

Why is this package name using - for separators?

Because the naming pattern for the intermediate packages is Microsoft.SourceBuild.Intermediate.. The repo name is literally source-build-reference-packages. This is a internal non-shipping infrastructure package.

Copy link
Member

@MichaelSimons MichaelSimons Jan 11, 2023

Choose a reason for hiding this comment

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

Friendly reminder @oleksandr-didyk to move this to be a product dependency.

Copy link
Member

@tmat tmat Jan 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why is this package name using - for separators?

Because the naming pattern for the intermediate packages is Microsoft.SourceBuild.Intermediate.. The repo name is literally source-build-reference-packages. This is a internal non-shipping infrastructure package.

Generally, we don't name packages based on name of repos. Not that this is super important, but the package could have been named Microsoft.SourceBuild.Intermediate.References or something like that.

Copy link
Contributor Author

@oleksandr-didyk oleksandr-didyk Jan 12, 2023

Choose a reason for hiding this comment

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

Friendly reminder @oleksandr-didyk to move this to be a product dependency.

My bad, committed the change but did not push. Thanks

tmat
tmat reviewed Jan 8, 2023
View reviewed changes
mmitche
mmitche reviewed Apr 3, 2023
View reviewed changes
eng/Versions.props Outdated
<UsingToolNetFrameworkReferenceAssemblies>true</UsingToolNetFrameworkReferenceAssemblies>
<UsingToolNuGetRepack>true</UsingToolNuGetRepack>
<!-- Dependencies -->
<MicrosoftSourceBuildIntermediatesourcebuildreferencepackagesPackageVersion>8.0.0-alpha.1.22616.1</MicrosoftSourceBuildIntermediatesourcebuildreferencepackagesPackageVersion>
Copy link
Member

@mmitche mmitche Apr 3, 2023

Choose a reason for hiding this comment

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

Suggested change
<MicrosoftSourceBuildIntermediatesourcebuildreferencepackagesPackageVersion>8.0.0-alpha.1.22616.1</MicrosoftSourceBuildIntermediatesourcebuildreferencepackagesPackageVersion>

@mmitche mmitche merged commit 0828c52 into dotnet:main Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tmat tmat tmat left review comments

@mmitche mmitche mmitche approved these changes

+1 more reviewer

@MichaelSimons MichaelSimons MichaelSimons approved these changes

Reviewers whose approvals may not affect merge requirements

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

4 participants

@oleksandr-didyk @tmat @MichaelSimons @mmitche