Skip to content

Conversation

@NikolaMilosavljevic
Copy link
Member

Summary of the changes

The goal is to enable per-repo source-build to detect prebuilt packages.

Fixes: #8491

@NikolaMilosavljevic NikolaMilosavljevic requested review from a team as code owners March 22, 2023 20:52
@NikolaMilosavljevic
Copy link
Member Author

@mmitche

<RoslynPackageVersion>4.6.0-3.23164.6</RoslynPackageVersion>
<VisualStudioLanguageServerProtocolVersion>17.6.4-preview</VisualStudioLanguageServerProtocolVersion>
<MicrosoftNetCompilersToolsetVersion>4.6.0-2.23128.3</MicrosoftNetCompilersToolsetVersion>
<MicrosoftNetCompilersToolsetVersion>$(RoslynPackageVersion)</MicrosoftNetCompilersToolsetVersion>
Copy link
Member

Choose a reason for hiding this comment

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

This change does not look correct. What's the purpose of it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this change should be conditioned for source-build only. It makes sense for Microsoft.Net.Compilers.Toolset package to be consumed from the same build as other roslyn packages, as it is built in roslyn. However, I don't know if this was an omission or intentional.

Copy link
Member Author

Choose a reason for hiding this comment

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

Source-build consumes packages from other repos, via intermediate packages, therefore we require that all packages from one repo have a matching version.

<UsagePattern IdentityGlob="Microsoft.SourceBuild.Intermediate.*/*" />

<!-- TODO:
Comment about missing sbrp packages and reference to tracking issue. -->
Copy link
Member

Choose a reason for hiding this comment

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

Missing comment?

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Mar 22, 2023

Choose a reason for hiding this comment

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

Comment is coming once I open the tracking issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed with f765403

<RoslynPackageVersion>4.6.0-3.23164.6</RoslynPackageVersion>
<VisualStudioLanguageServerProtocolVersion>17.6.4-preview</VisualStudioLanguageServerProtocolVersion>
<MicrosoftNetCompilersToolsetVersion>4.6.0-2.23128.3</MicrosoftNetCompilersToolsetVersion>
<MicrosoftNetCompilersToolsetVersion>$(RoslynPackageVersion)</MicrosoftNetCompilersToolsetVersion>
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this change should be conditioned for source-build only. It makes sense for Microsoft.Net.Compilers.Toolset package to be consumed from the same build as other roslyn packages, as it is built in roslyn. However, I don't know if this was an omission or intentional.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it should be conditioned. I'm not sure why M.N.C.T should diverge from the rest of the roslyn depss. The versions currently in there suggests that maybe this was an oversight in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

I know in VS Mac we have at times had to split roslyn dependency for IDE tooling, from roslyn version used to build, but those times were few and far between, and because of strange mono/mac/arm issues. I would hope we don't hit anything that exciting here, so keeping them in sync makes sense to me.

<Tooling_MicrosoftCodeAnalysisTestingVersion>1.1.2-beta1.22512.1</Tooling_MicrosoftCodeAnalysisTestingVersion>
<MicrosoftVisualStudioShellPackagesVersion>17.5.0-preview-2-33117-317</MicrosoftVisualStudioShellPackagesVersion>
<MicrosoftVisualStudioPackagesVersion>17.5.274-preview</MicrosoftVisualStudioPackagesVersion>
<RoslynPackageVersion>4.6.0-3.23164.6</RoslynPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

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

This should change to be a property matching the version.details.xml element you added: https://github.com/dotnet/razor/pull/8492/files#diff-fb62e94a1d6f29f863e3d0a22aa38269f6cd1d7f03b109dc06e2cbf2548b86d3R13

MicrosoftCodeAnalysisCSharpVersion

Copy link
Member Author

Choose a reason for hiding this comment

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

OK - I'll introduce the property, but also keep using RoslynPackageVersion as it's used in many places in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mmitche I've updated the dependency and tweaked the implementation a bit, in 049cc5d

I think this is now a bit cleaner.

<MicrosoftVisualStudioShellPackagesVersion>17.5.0-preview-2-33117-317</MicrosoftVisualStudioShellPackagesVersion>
<MicrosoftVisualStudioPackagesVersion>17.5.274-preview</MicrosoftVisualStudioPackagesVersion>
<RoslynPackageVersion>4.6.0-3.23164.6</RoslynPackageVersion>
<RoslynPackageVersion>$(MicrosoftNetCompilersToolsetVersion)</RoslynPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

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

We can't actually use proxy properties like this. Product source build won't be able to override RoslynPackageVersion since no package has that name. So, in source-build, any dependency using RoslynPackageVersion will retain its original value, leading to prebuilts.

So we need to remove RoslynPackageVersion and replace its use with MicrosoftNetCompilersToolsetVersion. If it is cleaner and you don't want to use MicrosoftNetCompilersToolsetVersion for, say Microsoft.CommonLanguageServer.Protocol.Framework, then you can add additional dependencies in eng/version.Details.xml/versions.props.

Copy link
Member

Choose a reason for hiding this comment

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

Hold up a second. I'm very concerned by the direction this change is going: we're talking about two completely unrelated concepts here. The toolset compiler is the version of the compiler that is used to build this repository. RoslynPackageVersion determines what Roslyn APIs the razor compiler and tooling expect when being run. Dynamically moving this forward for source-build would mean that we're compiling a configuration that we don't test.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't actually use proxy properties like this. Product source build won't be able to override RoslynPackageVersion since no package has that name. So, in source-build, any dependency using RoslynPackageVersion will retain its original value, leading to prebuilts.

How does this work today, is this already an issue? Bunch of properties are initialized with value of RoslynPackageVersion -

razor/eng/Versions.props

Lines 138 to 151 in 625b592

<MicrosoftCodeAnalysisExternalAccessRazorPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisExternalAccessRazorPackageVersion>
<MicrosoftCodeAnalysisExternalAccessOmniSharpCSharpPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisExternalAccessOmniSharpCSharpPackageVersion>
<MicrosoftCodeAnalysisCommonPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCommonPackageVersion>
<MicrosoftCodeAnalysisCSharpPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpPackageVersion>
<MicrosoftCodeAnalysisCSharpEditorFeaturesPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpEditorFeaturesPackageVersion>
<MicrosoftCodeAnalysisCSharpFeaturesPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpFeaturesPackageVersion>
<MicrosoftCodeAnalysisCSharpPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpPackageVersion>
<MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion>
<MicrosoftCodeAnalysisEditorFeaturesCommonPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisEditorFeaturesCommonPackageVersion>
<MicrosoftCodeAnalysisEditorFeaturesTextPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisEditorFeaturesTextPackageVersion>
<MicrosoftCodeAnalysisRemoteServiceHubPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisRemoteServiceHubPackageVersion>
<MicrosoftCodeAnalysisVisualBasicWorkspacesPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisVisualBasicWorkspacesPackageVersion>
<MicrosoftCodeAnalysisWorkspacesCommonPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisWorkspacesCommonPackageVersion>
<MicrosoftCodeAnalysisWorkspacesMSBuildPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisWorkspacesMSBuildPackageVersion>

Copy link
Member

Choose a reason for hiding this comment

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

Source-build doesn't really have an option to bifurcate these. There are essentially only two roslyn compilers which can be used:

  • The one coming in from previously source-built artifacts (i.e. previous SDK that was built).
  • The one built from the roslyn sources in the current build.

@333fred Why not simply use the same version for what you compile with and the surface area you're compiling against?

Copy link
Member

Choose a reason for hiding this comment

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

My concern is that the interaction between that version of roslyn's APIs and Razor has not been tested. I don't care so much about what version of the toolset compiler is used to compile Razor; I care much more deeply about what version of Roslyn we're referencing when we build.

Copy link
Member

Choose a reason for hiding this comment

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

Looking through a few of them, I think the properties are all used for PackageReferences that are excluded from the Linux source build.

Copy link
Member

Choose a reason for hiding this comment

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

So, in that case @NikolaMilosavljevic, we should leave the RoslynPackageVersion properties alone and use add a property/dependency for Microsoft.Net.Compilers.Toolset, like you did before.

If these projects ever get pulled into Linux source build, this will have to be resolved.

Copy link
Member

Choose a reason for hiding this comment

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

So essentially, source build is only overriding M.N.C.T version.

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Mar 23, 2023

Choose a reason for hiding this comment

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

Microsoft.CodeAnalysis.Common and Microsoft.CodeAnalysis.CSharp were showing as prebuilts until I synchronized $(RoslynPackageVersion) with $(MicrosoftNetCompilersToolsetVersion). As mentioned before, source-build needs to consume packages from a single roslyn intermediate package, so versions naturally have to be the same.

I'm not sure if repo source-build would produce different prebuilts from product source-build, but I suspect they should be exactly the same.

Copy link
Member

Choose a reason for hiding this comment

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

Can you root cause why that synchronization is necessary (where the package refs are used). They do have to be synchronized if the package refs show up in source build. But it looks like most of refs were in projects excluded from source build, which would not affect the pre-builts.

<MicrosoftCodeAnalysisCSharpPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpPackageVersion>
<!-- Microsoft.CodeAnalysis.Common and Microsoft.CodeAnalysis.CSharp packages are consumed in source-build.
Package version has to match roslyn repo reference as specified in MicrosoftNetCompilersToolsetVersion -->
<MicrosoftCodeAnalysisCommonPackageVersion>$(MicrosoftNetCompilersToolsetVersion)</MicrosoftCodeAnalysisCommonPackageVersion>
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 these are the right versions now. IMO it may be good to:

  • Rename RoslynPackageVersion to something like:
    "RoslynSurfaceAreaVersion" to better reflect its usage.
  • Add new dependencies to eng/Version.Details.xml for CodeAnalysis.Common and CodeAnalysis.CSharp and use their values explicitly here. Their versions would be the same as Microsoft.Net.Compilers.Toolset.

@mmitche
Copy link
Member

mmitche commented Mar 23, 2023

@333fred I want to a dig a little deeper here on razor's expected behavior and what is happening in source build.

The reason we don't see new prebuilts in source-build today is that RoslynPackageVersion is used to set MicrosoftCodeAnalysisCSharpPackageVersion (and other properties). Then those properties are used in the actual PackageReferences. In source-build, after eng/Versions.props is imported, we import the versions of roslyn that were built before razor, and that overrides these properties. So source-build has never been building razor against the roslyn that it is tested against in this repo.

The other thing to note is that when we layout the razor binaries into the non-source build .NET SDK, we do not deploy the Microsoft.CodeAnalysis.* version that razor is built against here. Instead, we use the roslyn version that flows into the SDK. Those are likely to be different.

Does it makes sense to simply remove RoslynPackageVersion altogether. Add individual dependencies to eng/Version.Details.xml for each of the places where it was used, and then have them all aligned. Therefore it will be very obvious if the toolset version and the roslyn surface area need to diverge, and source build will properly override them.

@333fred
Copy link
Member

333fred commented Mar 23, 2023

I don't think we can just remove RoslynPackageVersion altogether; I would be concerned about unintentionally introducing instability into the IDE tests by doing so. I'm going to start a teams thread on this.

@mmitche
Copy link
Member

mmitche commented Mar 23, 2023

I don't think we can just remove RoslynPackageVersion altogether; I would be concerned about unintentionally introducing instability into the IDE tests by doing so. I'm going to start a teams thread on this.

Okay sounds good. We should figure out what the desired behavior actually is, and how it may differ from what we have today in the various scenarios:

  • VS
  • SDK
  • Source-built SDK

If special properties are kept, they should be named well (e.g. RoslynSurfaceAreaVersion?), and we should add comments as to the expected behavior so that we don't have to deconstruct this again.

<!-- Ignoring the following prebuilts. We do not have SBRP packages for
7.0 targeting packs and System.Text.Encoding.CodePages.6.0.0 package.
https://github.com/dotnet/source-build/issues/3341 -->
<UsagePattern IdentityGlob="Microsoft.AspNetCore.App.Ref/7.0.2" />
Copy link
Member

Choose a reason for hiding this comment

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

The reason for the two 7.0 targetting packs should be tracked with #8293. We do not want these to be solved with SBRP.

Copy link
Member Author

Choose a reason for hiding this comment

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

The reason for the two 7.0 targetting packs should be tracked with #8293. We do not want these to be solved with SBRP.

Updated with 66c7617

Added a reference to reasons for net7.0 targeting and acreference to new tracking issue for net7.0 targeting removal.

https://github.com/dotnet/source-build/issues/3341 -->
<UsagePattern IdentityGlob="Microsoft.AspNetCore.App.Ref/7.0.2" />
<UsagePattern IdentityGlob="Microsoft.NETCore.App.Ref/7.0.2" />
<UsagePattern IdentityGlob="System.Text.Encoding.CodePages/6.0.0" />
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this can't be added to SBRP right away before this is PR is merged in?

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 reason this can't be added to SBRP right away before this is PR is merged in?

Good idea - I'll add System.Text.Encoding.CodePages/6.0.0 sbrp package and update the comment to reference the other issue for 7.0 targeting packs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will update this file after SBRP PR is merged - dotnet/source-build-reference-packages#591

<Uri>https://dev.azure.com/dnceng/internal/_git/dotnet-runtime</Uri>
<Sha>839cdfb0ecca5e0be3dbccd926e7651ef50fdf10</Sha>
</Dependency>
<Dependency Name="Microsoft.SourceBuild.Intermediate.source-build-reference-packages" Version="8.0.0-alpha.1.23159.2">
Copy link
Member

Choose a reason for hiding this comment

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

Have darc subscriptions been created for this and the new roslyn dependency?

Copy link
Member Author

Choose a reason for hiding this comment

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

Have darc subscriptions been created for this and the new roslyn dependency?

No, as I've noticed in some other per-repo prebuilt work, that subscriptions were created after PRs were merged. Perhaps it's irrelevant as it won't kick-in until these dependencies show up.

Do we also need coherency attributes for these new dependencies?

Copy link
Member

Choose a reason for hiding this comment

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

We do not. I also don't think we should create it for roslyn, unless it's set to 'none'. They try to match the roslyn that exists in VS.

Copy link
Member

Choose a reason for hiding this comment

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

My wording choice was poor. I was really asking if you were intending to define one. It feels like an SBRP subscription would be valuable.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, we should create one for SBRP.

Copy link
Member Author

@NikolaMilosavljevic NikolaMilosavljevic Apr 10, 2023

Choose a reason for hiding this comment

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

I'm going to create a subscription for SBRP - should it be a daily or weekly subscription? I presume we don't want it to be manual.

@mmitche @MichaelSimons

@NikolaMilosavljevic
Copy link
Member Author

@333fred @mmitche there were some initial concerns with direction of this work and some of the changes. Was everything addressed?

Comment on lines 138 to 148
<MicrosoftCodeAnalysisExternalAccessRazorPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisExternalAccessRazorPackageVersion>
<MicrosoftCodeAnalysisExternalAccessOmniSharpCSharpPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisExternalAccessOmniSharpCSharpPackageVersion>
<MicrosoftCodeAnalysisCommonPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCommonPackageVersion>
<MicrosoftCodeAnalysisCSharpPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpPackageVersion>
<!-- Microsoft.CodeAnalysis.Common and Microsoft.CodeAnalysis.CSharp packages are consumed in source-build.
Package version has to match roslyn repo reference as specified in MicrosoftNetCompilersToolsetVersion -->
<MicrosoftCodeAnalysisCommonPackageVersion>$(MicrosoftNetCompilersToolsetVersion)</MicrosoftCodeAnalysisCommonPackageVersion>
<MicrosoftCodeAnalysisCSharpPackageVersion>$(MicrosoftNetCompilersToolsetVersion)</MicrosoftCodeAnalysisCSharpPackageVersion>
<MicrosoftCodeAnalysisCSharpEditorFeaturesPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpEditorFeaturesPackageVersion>
<MicrosoftCodeAnalysisCSharpFeaturesPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpFeaturesPackageVersion>
<MicrosoftCodeAnalysisCSharpPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpPackageVersion>
<MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisCSharpWorkspacesPackageVersion>
<MicrosoftCodeAnalysisEditorFeaturesCommonPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisEditorFeaturesCommonPackageVersion>
<MicrosoftCodeAnalysisEditorFeaturesTextPackageVersion>$(RoslynPackageVersion)</MicrosoftCodeAnalysisEditorFeaturesTextPackageVersion>
Copy link
Member

Choose a reason for hiding this comment

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

These all need to be the same version. The likelihood is really that we can entirely remove MicrosoftNetCompilersToolsetVersion and just make everything RoslynPackageVersion, and then update from source-build as needed. I'm still not super comfortable with that, but I think that if we move this version to being auto-updated by darc on a regular cadence that would ameliorate much of my concern about shipping a different product in source-build and everywhere else.

Copy link
Member Author

Choose a reason for hiding this comment

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

These all need to be the same version. The likelihood is really that we can entirely remove MicrosoftNetCompilersToolsetVersion and just make everything RoslynPackageVersion, and then update from source-build as needed. I'm still not super comfortable with that, but I think that if we move this version to being auto-updated by darc on a regular cadence that would ameliorate much of my concern about shipping a different product in source-build and everywhere else.

Source-build does not know about repo-specific meta properties like RoslynPackageVersion. Source-build needs to update versions of all packages consumed during-source build, to ensure that versions are correct and all dependent packages are available in offline package cache.

@mmitche is there anything we haven't tried to reconcile the differing requirements by razor's regular- and source- builds? We could set some of these properties differently, only in source-build, i.e. MicrosoftCodeAnalysisCommonPackageVersion and MicrosoftCodeAnalysisCSharpPackageVersion.

Copy link
Member

Choose a reason for hiding this comment

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

Source-build does not know about repo-specific meta properties like RoslynPackageVersion.

Sure, but it all needs to be the same version. There cannot be differing input versions or we're going to run into build problems.

Copy link
Member

Choose a reason for hiding this comment

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

Right, agreed. So I think you basically want to:

  • Remove RoslynPackageVersion, since it can't be updated by Maestro. Today this is updated manually.
  • Instead, use the properties that correspond to all the individual packages, and simply set their versions to be all identical. All of these dependencies should go into Version.Details.xml, or else source build will not properly override them with PVP flow.
  • Then, @333fred and his team decide on the build of roslyn that gets used in this repo. An update just sets all the package versions. That could either be done via darc update-dependencies manually, or via regular updates from roslyn through Maestro.

So, you get the same behavior today you have with RoslynPackageVersion, a predictable version. It just also gets used to compile the repo via MicrosoftNETCompilersToolsetPackageVersion.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds good. We've been talking about having things flow through Maestro (to eliminate the problem of source-build building a product we never test), but that setup will probably wait until after I'm back from vacation. For right now, the version being used in RoslynPackageVersion should work.

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed the changes to address this: 647f22f. I think it should be ok now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Code-check is failing - will address it soon. Here are the errors:

2023-04-14T17:31:15.1977631Z error : Missing version variable 'MicrosoftSourceBuildIntermediatesourcebuildreferencepackagesPackageVersion' in the 'Automated' property group in D:\a\_work\1\s/eng/Versions.props
2023-04-14T17:31:15.1982203Z error : Missing version variable 'MicrosoftNetCompilersToolsetPackageVersion' in the 'Automated' property group in D:\a\_work\1\s/eng/Versions.props
2023-04-14T17:31:15.1986576Z error : Missing version variable 'MicrosoftCommonLanguageServerProtocolFrameworkPackageVersion' in the 'Automated' property group in D:\a\_work\1\s/eng/Versions.props
2023-04-14T17:31:15.1991069Z error : Missing version variable 'MicrosoftSourceLinkGitHubPackageVersion' in the 'Automated' property group in D:\a\_work\1\s/eng/Versions.props
2023-04-14T17:31:15.1995497Z error : Missing version variable 'MicrosoftDotNetXliffTasksPackageVersion' in the 'Automated' property group in D:\a\_work\1\s/eng/Versions.props
2023-04-14T17:31:15.2000084Z error : Version variable 'MicrosoftNetCompilersToolsetVersion' does not have a matching entry in Version.Details.xml. See https://github.com/dotnet/aspnetcore/blob/main/docs/ReferenceResolution.md for instructions on how to add a new dependency.

Copy link
Member Author

Choose a reason for hiding this comment

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

Code-check is clean now: 1f8ba5e

Copy link
Member Author

Choose a reason for hiding this comment

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

@333fred @mmitche I implemented the changes as discussed above. Can you take another look and approve?

Copy link
Member

@333fred 333fred left a comment

Choose a reason for hiding this comment

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

@dotnet/razor-tooling for a look at this as well.

Copy link
Member

@davidwengier davidwengier left a comment

Choose a reason for hiding this comment

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

Changes look okay. Can someone summarize where we are for Roslyn dependencies here now?

  • Are we allowed to manually bump the version of Roslyn being used, or would that break source build?
  • If no, are we now getting automated Roslyn bumps through arcade?

@NikolaMilosavljevic
Copy link
Member Author

Changes look okay. Can someone summarize where we are for Roslyn dependencies here now?

  • Are we allowed to manually bump the version of Roslyn being used, or would that break source build?
  • If no, are we now getting automated Roslyn bumps through arcade?

Full, product, source-build would set it's own versions for all packages - it would use live versions of all packages, including roslyn. Source-build leg in razor repo would use packages that are specified in Versions.props file.

You can update versions of all Roslyn packages either manually or automatically, by triggering Maestro subscription. @mmitche I presume there is a way to trigger subscription for one repo only, with a specific version number, is that correct? Or, does Maestro always updates to latest?

Either way, manual update is always possible, you'd simply update package versions in Version.Details.xml and Versions.props.

@NikolaMilosavljevic
Copy link
Member Author

I've resolved the merge conflict - there is a mismatch in versions of new dependencies - working on updating those.

Once this PR is merged, there won't be similar issues due to CoherentParentDependency tags.

@NikolaMilosavljevic
Copy link
Member Author

Checks are hitting some GitHub access issues... Will retry later.

@NikolaMilosavljevic
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 8492 in repo dotnet/razor

@davidwengier
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@davidwengier
Copy link
Member

davidwengier commented Apr 25, 2023

The unit test failure should be fixed by merging in main. Looks like there is one real failure though nvm, its a conflict from an arcade update.

@NikolaMilosavljevic
Copy link
Member Author

The unit test failure should be fixed by merging in main. Looks like there is one real failure though nvm, its a conflict from an arcade update.

I'll resolve the merge conflict and any fallout issues, likely resulting from lack of coherency in razor dependencies - this PR is adding the coherency.

@NikolaMilosavljevic
Copy link
Member Author

The failures seem related to some access or permission issues - we'll need to request a rerun a bit later.

@NikolaMilosavljevic
Copy link
Member Author

The unit test failure should be fixed by merging in main. Looks like there is one real failure though nvm, its a conflict from an arcade update.

One of the tests timed out - is it possible to rerun that check?

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.

Enable source-build pre-built detection

6 participants