Remove date number from dev build version

[dagood]

Fixes: #1089

[safern]

Can we leave this set to true when we're in an Official build? So that package versions are unique?

[dagood]

Not necessary. https://github.com/dotnet/arcade/blob/bca68c2df2c6230389f10def3880a61db266c6c1/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L39

[safern]

Awesome, arcade is ahead of me 😄

[dagood]

Getting errors in Libraries build--a lot of these:

.packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.20063.2\build\Packaging.targets(1161,5): error : File Microsoft.Win32.Primitives, version 42.42.42.42 was included framework package Microsoft.Private.CoreFx.NETCoreApp/5.0.0-ci but that version is not considered inbox in package index F:\workspace_work\1\s\src\libraries\pkg\Microsoft.Private.PackageBaseline\packageIndex.json. Please add it with appropriate InboxOn entry for netcoreapp5.0 or suppress this message with PermitInbox suppression.

[safern]

Getting errors in Libraries build--a lot of these:

Yeah, the assembly version was affected by this change and now it is getting arcade's 42.42.42.42 version... we need to figure out how to set it up so that it respects our AssemblyVersions.

[dagood]

Might be tricky, getting Arcade to produce date-based assembly versions but not use the date for the packages. Maybe condition DotNetUseShippingVersions so it's specifically on during certain parts of the build? Don't know if that'll work out.

Feel free to push to this PR if you have time to work on it (or make a new one, no real difference).

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

[safern]

cc: @tmat@mmitche for discussion.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

Never mind I think we figured out the issue.

[dagood]

This matches https://github.com/dotnet/corefx/pull/41723/files#diff-8b8f08ffbf7b863fb3700c1718eeb4cbR5, we should maybe use ProductVersion or Major/Minor/PatchVersion though.

[safern]

cc: @ericstj I think it makes sense to use any of the recommendations above.

[tmat]

Yes, we should not have the version specified in multiple places.
I'd use <AssemblyVersion>$(MajorVersion).$(MinorVersion).$(PatchVersion).0</AssemblyVersion> if you want to suppress defaulting to 42.42.42.42.

[tmat]

I'd be also nice to add a comment that explains why is this set here - something like (guessing) "The AssemblyVersion must match netcoreappM.N moniker. Whenever the version is updated we need to update the TFM as well.".

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

To expand on this a bit (in case the assembly version fix doesn't work, or maybe something else breaks downstream), maybe we can keep DotNetUseShippingVersions as true, but set /p:_BuildNumber=$(BUILD_BUILDNUMBER) so that the version stays constant across the jobs.

[safern]

That might be a good idea, the problem I see with that is that it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

[safern]

It seems like the dependency to System.Private.CoreLib has the wrong version, I don't know if CoreLib also needs to be 5.0.0.0 or if we're calculating the dependency wrong. @ericstj might know.

C:\h\w\98B8088C\p\packageTest.targets(74,5): error : Assembly 'System.Runtime.Intrinsics.Experimental' has insufficient version for dependency 'System.Private.CoreLib' : 5.0.0.0 < 42.42.42.42. [C:\h\w\98B8088C\w\A0E70914\u\netcoreapp5.0\project.csproj]

[mmitche]

What's the context here? I think I'm missing what the root of the issue is.

[dagood]

See #1089

[tmat]

@dagood Are there any customizations to versioning in dotnet/runtime repo that tweak the default Arcade versions or are there any parts of the repo that use something different than the versions Arcade sets? I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement). The build should not depend on current time in any way. If there is some part of the build that depends on current date/time that'd be a bug.

[dagood]

I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement).

PR builds are not official builds, so we don't use OfficialBuildId. The Arcade docs include this guidance: Versioning.md#build-kind.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

[dagood]

As for why we have this issue:

it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

[tmat]

PR builds are not official builds, so we don't use OfficialBuildId.

I see. I mistaken this for official build issue. Got it. In PR validation though the version has Major.Minor.Path-ci format. No date based version number is added.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

I would rather avoid that. _BuildNumber is a private property not to be used outside of Arcade SDK targets.

[tmat]

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

Now I understand. Yes, don't set this.

[dagood]

You don't have to tell me. 😄

[safern]

I think we just need to figure out why we're getting the package test failures. I agree we shouldn't set DotNetUseShippingVersions. I'll take a look locally to see if I get a repro and a suggested fix.

[dagood]

Please feel free to push to this PR or make your own, I'm not actively working on this.

[safern]

@dagood I moved the AssemblyVersion declaration to Versions.props and also, I made it use $(MajorVersion).$(MinorVersion).0.0 -- as for the patches and revisions should be manually updated per assembly if it is serviced (talked to @ericstj on this).

Also, package testing is currently using a Microsoft.NETCore.App package from our custom feeds. I updated it manually to use the latest one. I didn't want to add a BAR subscription to the runtime repo because of the ongoing discussion regarding to that and we should eventually fix package testing to use the live bits rather than the latest published bits. Will open an issue for that.

[dagood]

Also mention this? My first thought looking at this line is why it doesn't use patch version.

as for the patches and revisions should be manually updated per assembly if it is serviced

[safern]

Sure. will update the comment.

[dagood]

I'm concerned that this version is being used... the installer tests need to be running on the current build's outputs or else they aren't covering anything. 😕 Can you link the issue you mentioned you'd file for this?

[safern]

I'm surprised the installer tests use this property, I thought this property was used by package testing only.

I mentioned it here:
#1823 (comment)

Could you elaborate how the installer tests use this instead of the recently built product?

[dagood]

Could you elaborate how the installer tests use this instead of the recently built product?

No, I'm not familiar with it and I'm surprised by it when you mentioned you had to change this to make the installer tests work.

[safern]

I didn't mean installer tests, I mean libraries all configuration package tests... we run some tests on our oob packages to make sure closure is complete and that we don't ship duplicate types and that all supported rids for those packages publish correctly, etc.

[dagood]

Ah. This should be moved out from <!-- Installer dependencies --> then, it seems to me. Thanks for the background.

[safern]

I see, that was a result from the repo merge as in corefx it was "core-setup dependencies" maybe when we consolidated it was just a find and replace from "core-setup" to "installer". Will remove.

[safern]

Actually, what it means is that those dependencies as built by the installer. If you look the file, those comments split the sections on where are those packages coming from, for example look at runtime-assets dependencies which comes from https://github.com/dotnet/runtime-assets or arcade dependencies.

[dagood]

Oh, that makes sense. Sorry for the confusion on this, no need to change it.

[dagood]

I don't have enough context on the test expectations to help with the current CI error:

Assert.Equal() Failure
↓ (pos 20)
Expected: ComLibrary, Version=1.0.0.0, Culture=neutral, PublicKeyToken=···
Actual: ComLibrary, Version=5.0.0.0, Culture=neutral, PublicKeyToken=···
↑ (pos 20)
at Microsoft.NET.HostModel.ComHost.Tests.ClsidMapTests.PublicComVisibleTypeWithGuidAdded() in /_/src/installer/test/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.ComHost.Tests/ClsidMapTests.cs:line 35

Maybe @vitek-karas and @elinor-fung can help?

[safern]

Thanks @dagood -- I'm already looking at this locally, getting a repro and then see why ComLibrary is not getting the right AssemblyVersion.

[elinor-fung]

The test checks creating of a file (used for COM support) with data about a library - reading the assembly metadata; it uses the output assembly of the ComLibrary project (https://github.com/dotnet/runtime/tree/master/src/installer/test/Assets/TestProjects/ComLibrary) reading the assembly metadata.

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

[safern]

thanks for explaining @elinor-fung 😄

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

Me too, that''s why now I'm trying this locally to figure out why it didn't honor my AssemblyVersion in the props file for the test projects.

[safern]

This is now ready 😄

[dagood]

LGTM. (Can't approve "my" PR. 🙂)

[dagood]

Fixes: #1089

[safern]

Can we leave this set to true when we're in an Official build? So that package versions are unique?

[dagood]

Not necessary. https://github.com/dotnet/arcade/blob/bca68c2df2c6230389f10def3880a61db266c6c1/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L39

[safern]

Awesome, arcade is ahead of me 😄

[dagood]

Getting errors in Libraries build--a lot of these:

.packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.20063.2\build\Packaging.targets(1161,5): error : File Microsoft.Win32.Primitives, version 42.42.42.42 was included framework package Microsoft.Private.CoreFx.NETCoreApp/5.0.0-ci but that version is not considered inbox in package index F:\workspace_work\1\s\src\libraries\pkg\Microsoft.Private.PackageBaseline\packageIndex.json. Please add it with appropriate InboxOn entry for netcoreapp5.0 or suppress this message with PermitInbox suppression.

[safern]

Getting errors in Libraries build--a lot of these:

Yeah, the assembly version was affected by this change and now it is getting arcade's 42.42.42.42 version... we need to figure out how to set it up so that it respects our AssemblyVersions.

[dagood]

Might be tricky, getting Arcade to produce date-based assembly versions but not use the date for the packages. Maybe condition DotNetUseShippingVersions so it's specifically on during certain parts of the build? Don't know if that'll work out.

Feel free to push to this PR if you have time to work on it (or make a new one, no real difference).

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

[safern]

cc: @tmat@mmitche for discussion.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

Never mind I think we figured out the issue.

[dagood]

This matches https://github.com/dotnet/corefx/pull/41723/files#diff-8b8f08ffbf7b863fb3700c1718eeb4cbR5, we should maybe use ProductVersion or Major/Minor/PatchVersion though.

[safern]

cc: @ericstj I think it makes sense to use any of the recommendations above.

[tmat]

Yes, we should not have the version specified in multiple places.
I'd use <AssemblyVersion>$(MajorVersion).$(MinorVersion).$(PatchVersion).0</AssemblyVersion> if you want to suppress defaulting to 42.42.42.42.

[tmat]

I'd be also nice to add a comment that explains why is this set here - something like (guessing) "The AssemblyVersion must match netcoreappM.N moniker. Whenever the version is updated we need to update the TFM as well.".

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

To expand on this a bit (in case the assembly version fix doesn't work, or maybe something else breaks downstream), maybe we can keep DotNetUseShippingVersions as true, but set /p:_BuildNumber=$(BUILD_BUILDNUMBER) so that the version stays constant across the jobs.

[safern]

That might be a good idea, the problem I see with that is that it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

[safern]

It seems like the dependency to System.Private.CoreLib has the wrong version, I don't know if CoreLib also needs to be 5.0.0.0 or if we're calculating the dependency wrong. @ericstj might know.

C:\h\w\98B8088C\p\packageTest.targets(74,5): error : Assembly 'System.Runtime.Intrinsics.Experimental' has insufficient version for dependency 'System.Private.CoreLib' : 5.0.0.0 < 42.42.42.42. [C:\h\w\98B8088C\w\A0E70914\u\netcoreapp5.0\project.csproj]

[mmitche]

What's the context here? I think I'm missing what the root of the issue is.

[dagood]

See #1089

[tmat]

@dagood Are there any customizations to versioning in dotnet/runtime repo that tweak the default Arcade versions or are there any parts of the repo that use something different than the versions Arcade sets? I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement). The build should not depend on current time in any way. If there is some part of the build that depends on current date/time that'd be a bug.

[dagood]

I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement).

PR builds are not official builds, so we don't use OfficialBuildId. The Arcade docs include this guidance: Versioning.md#build-kind.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

[dagood]

As for why we have this issue:

it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

[tmat]

PR builds are not official builds, so we don't use OfficialBuildId.

I see. I mistaken this for official build issue. Got it. In PR validation though the version has Major.Minor.Path-ci format. No date based version number is added.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

I would rather avoid that. _BuildNumber is a private property not to be used outside of Arcade SDK targets.

[tmat]

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

Now I understand. Yes, don't set this.

[dagood]

You don't have to tell me. 😄

[safern]

I think we just need to figure out why we're getting the package test failures. I agree we shouldn't set DotNetUseShippingVersions. I'll take a look locally to see if I get a repro and a suggested fix.

[dagood]

Please feel free to push to this PR or make your own, I'm not actively working on this.

[safern]

@dagood I moved the AssemblyVersion declaration to Versions.props and also, I made it use $(MajorVersion).$(MinorVersion).0.0 -- as for the patches and revisions should be manually updated per assembly if it is serviced (talked to @ericstj on this).

Also, package testing is currently using a Microsoft.NETCore.App package from our custom feeds. I updated it manually to use the latest one. I didn't want to add a BAR subscription to the runtime repo because of the ongoing discussion regarding to that and we should eventually fix package testing to use the live bits rather than the latest published bits. Will open an issue for that.

[dagood]

Also mention this? My first thought looking at this line is why it doesn't use patch version.

as for the patches and revisions should be manually updated per assembly if it is serviced

[safern]

Sure. will update the comment.

[dagood]

I'm concerned that this version is being used... the installer tests need to be running on the current build's outputs or else they aren't covering anything. 😕 Can you link the issue you mentioned you'd file for this?

[safern]

I'm surprised the installer tests use this property, I thought this property was used by package testing only.

I mentioned it here:
#1823 (comment)

Could you elaborate how the installer tests use this instead of the recently built product?

[dagood]

Could you elaborate how the installer tests use this instead of the recently built product?

No, I'm not familiar with it and I'm surprised by it when you mentioned you had to change this to make the installer tests work.

[safern]

I didn't mean installer tests, I mean libraries all configuration package tests... we run some tests on our oob packages to make sure closure is complete and that we don't ship duplicate types and that all supported rids for those packages publish correctly, etc.

[dagood]

Ah. This should be moved out from <!-- Installer dependencies --> then, it seems to me. Thanks for the background.

[safern]

I see, that was a result from the repo merge as in corefx it was "core-setup dependencies" maybe when we consolidated it was just a find and replace from "core-setup" to "installer". Will remove.

[safern]

Actually, what it means is that those dependencies as built by the installer. If you look the file, those comments split the sections on where are those packages coming from, for example look at runtime-assets dependencies which comes from https://github.com/dotnet/runtime-assets or arcade dependencies.

[dagood]

Oh, that makes sense. Sorry for the confusion on this, no need to change it.

[dagood]

I don't have enough context on the test expectations to help with the current CI error:

Assert.Equal() Failure
↓ (pos 20)
Expected: ComLibrary, Version=1.0.0.0, Culture=neutral, PublicKeyToken=···
Actual: ComLibrary, Version=5.0.0.0, Culture=neutral, PublicKeyToken=···
↑ (pos 20)
at Microsoft.NET.HostModel.ComHost.Tests.ClsidMapTests.PublicComVisibleTypeWithGuidAdded() in /_/src/installer/test/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.ComHost.Tests/ClsidMapTests.cs:line 35

Maybe @vitek-karas and @elinor-fung can help?

[safern]

Thanks @dagood -- I'm already looking at this locally, getting a repro and then see why ComLibrary is not getting the right AssemblyVersion.

[elinor-fung]

The test checks creating of a file (used for COM support) with data about a library - reading the assembly metadata; it uses the output assembly of the ComLibrary project (https://github.com/dotnet/runtime/tree/master/src/installer/test/Assets/TestProjects/ComLibrary) reading the assembly metadata.

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

[safern]

thanks for explaining @elinor-fung 😄

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

Me too, that''s why now I'm trying this locally to figure out why it didn't honor my AssemblyVersion in the props file for the test projects.

[safern]

This is now ready 😄

[dagood]

LGTM. (Can't approve "my" PR. 🙂)

[dagood]

Fixes: #1089

[safern]

Can we leave this set to true when we're in an Official build? So that package versions are unique?

[dagood]

Not necessary. https://github.com/dotnet/arcade/blob/bca68c2df2c6230389f10def3880a61db266c6c1/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L39

[safern]

Awesome, arcade is ahead of me 😄

[dagood]

Getting errors in Libraries build--a lot of these:

.packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.20063.2\build\Packaging.targets(1161,5): error : File Microsoft.Win32.Primitives, version 42.42.42.42 was included framework package Microsoft.Private.CoreFx.NETCoreApp/5.0.0-ci but that version is not considered inbox in package index F:\workspace_work\1\s\src\libraries\pkg\Microsoft.Private.PackageBaseline\packageIndex.json. Please add it with appropriate InboxOn entry for netcoreapp5.0 or suppress this message with PermitInbox suppression.

[safern]

Getting errors in Libraries build--a lot of these:

Yeah, the assembly version was affected by this change and now it is getting arcade's 42.42.42.42 version... we need to figure out how to set it up so that it respects our AssemblyVersions.

[dagood]

Might be tricky, getting Arcade to produce date-based assembly versions but not use the date for the packages. Maybe condition DotNetUseShippingVersions so it's specifically on during certain parts of the build? Don't know if that'll work out.

Feel free to push to this PR if you have time to work on it (or make a new one, no real difference).

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

[safern]

cc: @tmat@mmitche for discussion.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

Never mind I think we figured out the issue.

[dagood]

This matches https://github.com/dotnet/corefx/pull/41723/files#diff-8b8f08ffbf7b863fb3700c1718eeb4cbR5, we should maybe use ProductVersion or Major/Minor/PatchVersion though.

[safern]

cc: @ericstj I think it makes sense to use any of the recommendations above.

[tmat]

Yes, we should not have the version specified in multiple places.
I'd use <AssemblyVersion>$(MajorVersion).$(MinorVersion).$(PatchVersion).0</AssemblyVersion> if you want to suppress defaulting to 42.42.42.42.

[tmat]

I'd be also nice to add a comment that explains why is this set here - something like (guessing) "The AssemblyVersion must match netcoreappM.N moniker. Whenever the version is updated we need to update the TFM as well.".

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

To expand on this a bit (in case the assembly version fix doesn't work, or maybe something else breaks downstream), maybe we can keep DotNetUseShippingVersions as true, but set /p:_BuildNumber=$(BUILD_BUILDNUMBER) so that the version stays constant across the jobs.

[safern]

That might be a good idea, the problem I see with that is that it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

[safern]

It seems like the dependency to System.Private.CoreLib has the wrong version, I don't know if CoreLib also needs to be 5.0.0.0 or if we're calculating the dependency wrong. @ericstj might know.

C:\h\w\98B8088C\p\packageTest.targets(74,5): error : Assembly 'System.Runtime.Intrinsics.Experimental' has insufficient version for dependency 'System.Private.CoreLib' : 5.0.0.0 < 42.42.42.42. [C:\h\w\98B8088C\w\A0E70914\u\netcoreapp5.0\project.csproj]

[mmitche]

What's the context here? I think I'm missing what the root of the issue is.

[dagood]

See #1089

[tmat]

@dagood Are there any customizations to versioning in dotnet/runtime repo that tweak the default Arcade versions or are there any parts of the repo that use something different than the versions Arcade sets? I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement). The build should not depend on current time in any way. If there is some part of the build that depends on current date/time that'd be a bug.

[dagood]

I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement).

PR builds are not official builds, so we don't use OfficialBuildId. The Arcade docs include this guidance: Versioning.md#build-kind.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

[dagood]

As for why we have this issue:

it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

[tmat]

PR builds are not official builds, so we don't use OfficialBuildId.

I see. I mistaken this for official build issue. Got it. In PR validation though the version has Major.Minor.Path-ci format. No date based version number is added.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

I would rather avoid that. _BuildNumber is a private property not to be used outside of Arcade SDK targets.

[tmat]

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

Now I understand. Yes, don't set this.

[dagood]

You don't have to tell me. 😄

[safern]

I think we just need to figure out why we're getting the package test failures. I agree we shouldn't set DotNetUseShippingVersions. I'll take a look locally to see if I get a repro and a suggested fix.

[dagood]

Please feel free to push to this PR or make your own, I'm not actively working on this.

[safern]

@dagood I moved the AssemblyVersion declaration to Versions.props and also, I made it use $(MajorVersion).$(MinorVersion).0.0 -- as for the patches and revisions should be manually updated per assembly if it is serviced (talked to @ericstj on this).

Also, package testing is currently using a Microsoft.NETCore.App package from our custom feeds. I updated it manually to use the latest one. I didn't want to add a BAR subscription to the runtime repo because of the ongoing discussion regarding to that and we should eventually fix package testing to use the live bits rather than the latest published bits. Will open an issue for that.

[dagood]

Also mention this? My first thought looking at this line is why it doesn't use patch version.

as for the patches and revisions should be manually updated per assembly if it is serviced

[safern]

Sure. will update the comment.

[dagood]

I'm concerned that this version is being used... the installer tests need to be running on the current build's outputs or else they aren't covering anything. 😕 Can you link the issue you mentioned you'd file for this?

[safern]

I'm surprised the installer tests use this property, I thought this property was used by package testing only.

I mentioned it here:
#1823 (comment)

Could you elaborate how the installer tests use this instead of the recently built product?

[dagood]

Could you elaborate how the installer tests use this instead of the recently built product?

No, I'm not familiar with it and I'm surprised by it when you mentioned you had to change this to make the installer tests work.

[safern]

I didn't mean installer tests, I mean libraries all configuration package tests... we run some tests on our oob packages to make sure closure is complete and that we don't ship duplicate types and that all supported rids for those packages publish correctly, etc.

[dagood]

Ah. This should be moved out from <!-- Installer dependencies --> then, it seems to me. Thanks for the background.

[safern]

I see, that was a result from the repo merge as in corefx it was "core-setup dependencies" maybe when we consolidated it was just a find and replace from "core-setup" to "installer". Will remove.

[safern]

Actually, what it means is that those dependencies as built by the installer. If you look the file, those comments split the sections on where are those packages coming from, for example look at runtime-assets dependencies which comes from https://github.com/dotnet/runtime-assets or arcade dependencies.

[dagood]

Oh, that makes sense. Sorry for the confusion on this, no need to change it.

[dagood]

I don't have enough context on the test expectations to help with the current CI error:

Assert.Equal() Failure
↓ (pos 20)
Expected: ComLibrary, Version=1.0.0.0, Culture=neutral, PublicKeyToken=···
Actual: ComLibrary, Version=5.0.0.0, Culture=neutral, PublicKeyToken=···
↑ (pos 20)
at Microsoft.NET.HostModel.ComHost.Tests.ClsidMapTests.PublicComVisibleTypeWithGuidAdded() in /_/src/installer/test/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.ComHost.Tests/ClsidMapTests.cs:line 35

Maybe @vitek-karas and @elinor-fung can help?

[safern]

Thanks @dagood -- I'm already looking at this locally, getting a repro and then see why ComLibrary is not getting the right AssemblyVersion.

[elinor-fung]

The test checks creating of a file (used for COM support) with data about a library - reading the assembly metadata; it uses the output assembly of the ComLibrary project (https://github.com/dotnet/runtime/tree/master/src/installer/test/Assets/TestProjects/ComLibrary) reading the assembly metadata.

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

[safern]

thanks for explaining @elinor-fung 😄

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

Me too, that''s why now I'm trying this locally to figure out why it didn't honor my AssemblyVersion in the props file for the test projects.

[safern]

This is now ready 😄

[dagood]

LGTM. (Can't approve "my" PR. 🙂)

[dagood]

Fixes: #1089

[safern]

Can we leave this set to true when we're in an Official build? So that package versions are unique?

[dagood]

Not necessary. https://github.com/dotnet/arcade/blob/bca68c2df2c6230389f10def3880a61db266c6c1/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L39

[safern]

Awesome, arcade is ahead of me 😄

[dagood]

Getting errors in Libraries build--a lot of these:

.packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.20063.2\build\Packaging.targets(1161,5): error : File Microsoft.Win32.Primitives, version 42.42.42.42 was included framework package Microsoft.Private.CoreFx.NETCoreApp/5.0.0-ci but that version is not considered inbox in package index F:\workspace_work\1\s\src\libraries\pkg\Microsoft.Private.PackageBaseline\packageIndex.json. Please add it with appropriate InboxOn entry for netcoreapp5.0 or suppress this message with PermitInbox suppression.

[safern]

Getting errors in Libraries build--a lot of these:

Yeah, the assembly version was affected by this change and now it is getting arcade's 42.42.42.42 version... we need to figure out how to set it up so that it respects our AssemblyVersions.

[dagood]

Might be tricky, getting Arcade to produce date-based assembly versions but not use the date for the packages. Maybe condition DotNetUseShippingVersions so it's specifically on during certain parts of the build? Don't know if that'll work out.

Feel free to push to this PR if you have time to work on it (or make a new one, no real difference).

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

[safern]

cc: @tmat@mmitche for discussion.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

Never mind I think we figured out the issue.

[dagood]

This matches https://github.com/dotnet/corefx/pull/41723/files#diff-8b8f08ffbf7b863fb3700c1718eeb4cbR5, we should maybe use ProductVersion or Major/Minor/PatchVersion though.

[safern]

cc: @ericstj I think it makes sense to use any of the recommendations above.

[tmat]

Yes, we should not have the version specified in multiple places.
I'd use <AssemblyVersion>$(MajorVersion).$(MinorVersion).$(PatchVersion).0</AssemblyVersion> if you want to suppress defaulting to 42.42.42.42.

[tmat]

I'd be also nice to add a comment that explains why is this set here - something like (guessing) "The AssemblyVersion must match netcoreappM.N moniker. Whenever the version is updated we need to update the TFM as well.".

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

To expand on this a bit (in case the assembly version fix doesn't work, or maybe something else breaks downstream), maybe we can keep DotNetUseShippingVersions as true, but set /p:_BuildNumber=$(BUILD_BUILDNUMBER) so that the version stays constant across the jobs.

[safern]

That might be a good idea, the problem I see with that is that it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

[safern]

It seems like the dependency to System.Private.CoreLib has the wrong version, I don't know if CoreLib also needs to be 5.0.0.0 or if we're calculating the dependency wrong. @ericstj might know.

C:\h\w\98B8088C\p\packageTest.targets(74,5): error : Assembly 'System.Runtime.Intrinsics.Experimental' has insufficient version for dependency 'System.Private.CoreLib' : 5.0.0.0 < 42.42.42.42. [C:\h\w\98B8088C\w\A0E70914\u\netcoreapp5.0\project.csproj]

[mmitche]

What's the context here? I think I'm missing what the root of the issue is.

[dagood]

See #1089

[tmat]

@dagood Are there any customizations to versioning in dotnet/runtime repo that tweak the default Arcade versions or are there any parts of the repo that use something different than the versions Arcade sets? I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement). The build should not depend on current time in any way. If there is some part of the build that depends on current date/time that'd be a bug.

[dagood]

I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement).

PR builds are not official builds, so we don't use OfficialBuildId. The Arcade docs include this guidance: Versioning.md#build-kind.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

[dagood]

As for why we have this issue:

it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

[tmat]

PR builds are not official builds, so we don't use OfficialBuildId.

I see. I mistaken this for official build issue. Got it. In PR validation though the version has Major.Minor.Path-ci format. No date based version number is added.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

I would rather avoid that. _BuildNumber is a private property not to be used outside of Arcade SDK targets.

[tmat]

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

Now I understand. Yes, don't set this.

[dagood]

You don't have to tell me. 😄

[safern]

I think we just need to figure out why we're getting the package test failures. I agree we shouldn't set DotNetUseShippingVersions. I'll take a look locally to see if I get a repro and a suggested fix.

[dagood]

Please feel free to push to this PR or make your own, I'm not actively working on this.

[safern]

@dagood I moved the AssemblyVersion declaration to Versions.props and also, I made it use $(MajorVersion).$(MinorVersion).0.0 -- as for the patches and revisions should be manually updated per assembly if it is serviced (talked to @ericstj on this).

Also, package testing is currently using a Microsoft.NETCore.App package from our custom feeds. I updated it manually to use the latest one. I didn't want to add a BAR subscription to the runtime repo because of the ongoing discussion regarding to that and we should eventually fix package testing to use the live bits rather than the latest published bits. Will open an issue for that.

[dagood]

Also mention this? My first thought looking at this line is why it doesn't use patch version.

as for the patches and revisions should be manually updated per assembly if it is serviced

[safern]

Sure. will update the comment.

[dagood]

I'm concerned that this version is being used... the installer tests need to be running on the current build's outputs or else they aren't covering anything. 😕 Can you link the issue you mentioned you'd file for this?

[safern]

I'm surprised the installer tests use this property, I thought this property was used by package testing only.

I mentioned it here:
#1823 (comment)

Could you elaborate how the installer tests use this instead of the recently built product?

[dagood]

Could you elaborate how the installer tests use this instead of the recently built product?

No, I'm not familiar with it and I'm surprised by it when you mentioned you had to change this to make the installer tests work.

[safern]

I didn't mean installer tests, I mean libraries all configuration package tests... we run some tests on our oob packages to make sure closure is complete and that we don't ship duplicate types and that all supported rids for those packages publish correctly, etc.

[dagood]

Ah. This should be moved out from <!-- Installer dependencies --> then, it seems to me. Thanks for the background.

[safern]

I see, that was a result from the repo merge as in corefx it was "core-setup dependencies" maybe when we consolidated it was just a find and replace from "core-setup" to "installer". Will remove.

[safern]

Actually, what it means is that those dependencies as built by the installer. If you look the file, those comments split the sections on where are those packages coming from, for example look at runtime-assets dependencies which comes from https://github.com/dotnet/runtime-assets or arcade dependencies.

[dagood]

Oh, that makes sense. Sorry for the confusion on this, no need to change it.

[dagood]

I don't have enough context on the test expectations to help with the current CI error:

Assert.Equal() Failure
↓ (pos 20)
Expected: ComLibrary, Version=1.0.0.0, Culture=neutral, PublicKeyToken=···
Actual: ComLibrary, Version=5.0.0.0, Culture=neutral, PublicKeyToken=···
↑ (pos 20)
at Microsoft.NET.HostModel.ComHost.Tests.ClsidMapTests.PublicComVisibleTypeWithGuidAdded() in /_/src/installer/test/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.ComHost.Tests/ClsidMapTests.cs:line 35

Maybe @vitek-karas and @elinor-fung can help?

[safern]

Thanks @dagood -- I'm already looking at this locally, getting a repro and then see why ComLibrary is not getting the right AssemblyVersion.

[elinor-fung]

The test checks creating of a file (used for COM support) with data about a library - reading the assembly metadata; it uses the output assembly of the ComLibrary project (https://github.com/dotnet/runtime/tree/master/src/installer/test/Assets/TestProjects/ComLibrary) reading the assembly metadata.

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

[safern]

thanks for explaining @elinor-fung 😄

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

Me too, that''s why now I'm trying this locally to figure out why it didn't honor my AssemblyVersion in the props file for the test projects.

[safern]

This is now ready 😄

[dagood]

LGTM. (Can't approve "my" PR. 🙂)

[dagood]

Fixes: #1089

[safern]

Can we leave this set to true when we're in an Official build? So that package versions are unique?

[dagood]

Not necessary. https://github.com/dotnet/arcade/blob/bca68c2df2c6230389f10def3880a61db266c6c1/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L39

[safern]

Awesome, arcade is ahead of me 😄

[dagood]

Getting errors in Libraries build--a lot of these:

.packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.20063.2\build\Packaging.targets(1161,5): error : File Microsoft.Win32.Primitives, version 42.42.42.42 was included framework package Microsoft.Private.CoreFx.NETCoreApp/5.0.0-ci but that version is not considered inbox in package index F:\workspace_work\1\s\src\libraries\pkg\Microsoft.Private.PackageBaseline\packageIndex.json. Please add it with appropriate InboxOn entry for netcoreapp5.0 or suppress this message with PermitInbox suppression.

[safern]

Getting errors in Libraries build--a lot of these:

Yeah, the assembly version was affected by this change and now it is getting arcade's 42.42.42.42 version... we need to figure out how to set it up so that it respects our AssemblyVersions.

[dagood]

Might be tricky, getting Arcade to produce date-based assembly versions but not use the date for the packages. Maybe condition DotNetUseShippingVersions so it's specifically on during certain parts of the build? Don't know if that'll work out.

Feel free to push to this PR if you have time to work on it (or make a new one, no real difference).

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

[safern]

cc: @tmat@mmitche for discussion.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

Never mind I think we figured out the issue.

[dagood]

This matches https://github.com/dotnet/corefx/pull/41723/files#diff-8b8f08ffbf7b863fb3700c1718eeb4cbR5, we should maybe use ProductVersion or Major/Minor/PatchVersion though.

[safern]

cc: @ericstj I think it makes sense to use any of the recommendations above.

[tmat]

Yes, we should not have the version specified in multiple places.
I'd use <AssemblyVersion>$(MajorVersion).$(MinorVersion).$(PatchVersion).0</AssemblyVersion> if you want to suppress defaulting to 42.42.42.42.

[tmat]

I'd be also nice to add a comment that explains why is this set here - something like (guessing) "The AssemblyVersion must match netcoreappM.N moniker. Whenever the version is updated we need to update the TFM as well.".

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

To expand on this a bit (in case the assembly version fix doesn't work, or maybe something else breaks downstream), maybe we can keep DotNetUseShippingVersions as true, but set /p:_BuildNumber=$(BUILD_BUILDNUMBER) so that the version stays constant across the jobs.

[safern]

That might be a good idea, the problem I see with that is that it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

[safern]

It seems like the dependency to System.Private.CoreLib has the wrong version, I don't know if CoreLib also needs to be 5.0.0.0 or if we're calculating the dependency wrong. @ericstj might know.

C:\h\w\98B8088C\p\packageTest.targets(74,5): error : Assembly 'System.Runtime.Intrinsics.Experimental' has insufficient version for dependency 'System.Private.CoreLib' : 5.0.0.0 < 42.42.42.42. [C:\h\w\98B8088C\w\A0E70914\u\netcoreapp5.0\project.csproj]

[mmitche]

What's the context here? I think I'm missing what the root of the issue is.

[dagood]

See #1089

[tmat]

@dagood Are there any customizations to versioning in dotnet/runtime repo that tweak the default Arcade versions or are there any parts of the repo that use something different than the versions Arcade sets? I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement). The build should not depend on current time in any way. If there is some part of the build that depends on current date/time that'd be a bug.

[dagood]

I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement).

PR builds are not official builds, so we don't use OfficialBuildId. The Arcade docs include this guidance: Versioning.md#build-kind.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

[dagood]

As for why we have this issue:

it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

[tmat]

PR builds are not official builds, so we don't use OfficialBuildId.

I see. I mistaken this for official build issue. Got it. In PR validation though the version has Major.Minor.Path-ci format. No date based version number is added.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

I would rather avoid that. _BuildNumber is a private property not to be used outside of Arcade SDK targets.

[tmat]

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

Now I understand. Yes, don't set this.

[dagood]

You don't have to tell me. 😄

[safern]

I think we just need to figure out why we're getting the package test failures. I agree we shouldn't set DotNetUseShippingVersions. I'll take a look locally to see if I get a repro and a suggested fix.

[dagood]

Please feel free to push to this PR or make your own, I'm not actively working on this.

[safern]

@dagood I moved the AssemblyVersion declaration to Versions.props and also, I made it use $(MajorVersion).$(MinorVersion).0.0 -- as for the patches and revisions should be manually updated per assembly if it is serviced (talked to @ericstj on this).

Also, package testing is currently using a Microsoft.NETCore.App package from our custom feeds. I updated it manually to use the latest one. I didn't want to add a BAR subscription to the runtime repo because of the ongoing discussion regarding to that and we should eventually fix package testing to use the live bits rather than the latest published bits. Will open an issue for that.

[dagood]

Also mention this? My first thought looking at this line is why it doesn't use patch version.

as for the patches and revisions should be manually updated per assembly if it is serviced

[safern]

Sure. will update the comment.

[dagood]

I'm concerned that this version is being used... the installer tests need to be running on the current build's outputs or else they aren't covering anything. 😕 Can you link the issue you mentioned you'd file for this?

[safern]

I'm surprised the installer tests use this property, I thought this property was used by package testing only.

I mentioned it here:
#1823 (comment)

Could you elaborate how the installer tests use this instead of the recently built product?

[dagood]

Could you elaborate how the installer tests use this instead of the recently built product?

No, I'm not familiar with it and I'm surprised by it when you mentioned you had to change this to make the installer tests work.

[safern]

I didn't mean installer tests, I mean libraries all configuration package tests... we run some tests on our oob packages to make sure closure is complete and that we don't ship duplicate types and that all supported rids for those packages publish correctly, etc.

[dagood]

Ah. This should be moved out from <!-- Installer dependencies --> then, it seems to me. Thanks for the background.

[safern]

I see, that was a result from the repo merge as in corefx it was "core-setup dependencies" maybe when we consolidated it was just a find and replace from "core-setup" to "installer". Will remove.

[safern]

Actually, what it means is that those dependencies as built by the installer. If you look the file, those comments split the sections on where are those packages coming from, for example look at runtime-assets dependencies which comes from https://github.com/dotnet/runtime-assets or arcade dependencies.

[dagood]

Oh, that makes sense. Sorry for the confusion on this, no need to change it.

[dagood]

I don't have enough context on the test expectations to help with the current CI error:

Assert.Equal() Failure
↓ (pos 20)
Expected: ComLibrary, Version=1.0.0.0, Culture=neutral, PublicKeyToken=···
Actual: ComLibrary, Version=5.0.0.0, Culture=neutral, PublicKeyToken=···
↑ (pos 20)
at Microsoft.NET.HostModel.ComHost.Tests.ClsidMapTests.PublicComVisibleTypeWithGuidAdded() in /_/src/installer/test/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.ComHost.Tests/ClsidMapTests.cs:line 35

Maybe @vitek-karas and @elinor-fung can help?

[safern]

Thanks @dagood -- I'm already looking at this locally, getting a repro and then see why ComLibrary is not getting the right AssemblyVersion.

[elinor-fung]

The test checks creating of a file (used for COM support) with data about a library - reading the assembly metadata; it uses the output assembly of the ComLibrary project (https://github.com/dotnet/runtime/tree/master/src/installer/test/Assets/TestProjects/ComLibrary) reading the assembly metadata.

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

[safern]

thanks for explaining @elinor-fung 😄

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

Me too, that''s why now I'm trying this locally to figure out why it didn't honor my AssemblyVersion in the props file for the test projects.

[safern]

This is now ready 😄

[dagood]

LGTM. (Can't approve "my" PR. 🙂)

[dagood]

Fixes: #1089

[safern]

Can we leave this set to true when we're in an Official build? So that package versions are unique?

[dagood]

Not necessary. https://github.com/dotnet/arcade/blob/bca68c2df2c6230389f10def3880a61db266c6c1/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L39

[safern]

Awesome, arcade is ahead of me 😄

[dagood]

Getting errors in Libraries build--a lot of these:

.packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.20063.2\build\Packaging.targets(1161,5): error : File Microsoft.Win32.Primitives, version 42.42.42.42 was included framework package Microsoft.Private.CoreFx.NETCoreApp/5.0.0-ci but that version is not considered inbox in package index F:\workspace_work\1\s\src\libraries\pkg\Microsoft.Private.PackageBaseline\packageIndex.json. Please add it with appropriate InboxOn entry for netcoreapp5.0 or suppress this message with PermitInbox suppression.

[safern]

Getting errors in Libraries build--a lot of these:

Yeah, the assembly version was affected by this change and now it is getting arcade's 42.42.42.42 version... we need to figure out how to set it up so that it respects our AssemblyVersions.

[dagood]

Might be tricky, getting Arcade to produce date-based assembly versions but not use the date for the packages. Maybe condition DotNetUseShippingVersions so it's specifically on during certain parts of the build? Don't know if that'll work out.

Feel free to push to this PR if you have time to work on it (or make a new one, no real difference).

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

[safern]

cc: @tmat@mmitche for discussion.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

Never mind I think we figured out the issue.

[dagood]

This matches https://github.com/dotnet/corefx/pull/41723/files#diff-8b8f08ffbf7b863fb3700c1718eeb4cbR5, we should maybe use ProductVersion or Major/Minor/PatchVersion though.

[safern]

cc: @ericstj I think it makes sense to use any of the recommendations above.

[tmat]

Yes, we should not have the version specified in multiple places.
I'd use <AssemblyVersion>$(MajorVersion).$(MinorVersion).$(PatchVersion).0</AssemblyVersion> if you want to suppress defaulting to 42.42.42.42.

[tmat]

I'd be also nice to add a comment that explains why is this set here - something like (guessing) "The AssemblyVersion must match netcoreappM.N moniker. Whenever the version is updated we need to update the TFM as well.".

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

To expand on this a bit (in case the assembly version fix doesn't work, or maybe something else breaks downstream), maybe we can keep DotNetUseShippingVersions as true, but set /p:_BuildNumber=$(BUILD_BUILDNUMBER) so that the version stays constant across the jobs.

[safern]

That might be a good idea, the problem I see with that is that it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

[safern]

It seems like the dependency to System.Private.CoreLib has the wrong version, I don't know if CoreLib also needs to be 5.0.0.0 or if we're calculating the dependency wrong. @ericstj might know.

C:\h\w\98B8088C\p\packageTest.targets(74,5): error : Assembly 'System.Runtime.Intrinsics.Experimental' has insufficient version for dependency 'System.Private.CoreLib' : 5.0.0.0 < 42.42.42.42. [C:\h\w\98B8088C\w\A0E70914\u\netcoreapp5.0\project.csproj]

[mmitche]

What's the context here? I think I'm missing what the root of the issue is.

[dagood]

See #1089

[tmat]

@dagood Are there any customizations to versioning in dotnet/runtime repo that tweak the default Arcade versions or are there any parts of the repo that use something different than the versions Arcade sets? I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement). The build should not depend on current time in any way. If there is some part of the build that depends on current date/time that'd be a bug.

[dagood]

I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement).

PR builds are not official builds, so we don't use OfficialBuildId. The Arcade docs include this guidance: Versioning.md#build-kind.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

[dagood]

As for why we have this issue:

it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

[tmat]

PR builds are not official builds, so we don't use OfficialBuildId.

I see. I mistaken this for official build issue. Got it. In PR validation though the version has Major.Minor.Path-ci format. No date based version number is added.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

I would rather avoid that. _BuildNumber is a private property not to be used outside of Arcade SDK targets.

[tmat]

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

Now I understand. Yes, don't set this.

[dagood]

You don't have to tell me. 😄

[safern]

I think we just need to figure out why we're getting the package test failures. I agree we shouldn't set DotNetUseShippingVersions. I'll take a look locally to see if I get a repro and a suggested fix.

[dagood]

Please feel free to push to this PR or make your own, I'm not actively working on this.

[safern]

@dagood I moved the AssemblyVersion declaration to Versions.props and also, I made it use $(MajorVersion).$(MinorVersion).0.0 -- as for the patches and revisions should be manually updated per assembly if it is serviced (talked to @ericstj on this).

Also, package testing is currently using a Microsoft.NETCore.App package from our custom feeds. I updated it manually to use the latest one. I didn't want to add a BAR subscription to the runtime repo because of the ongoing discussion regarding to that and we should eventually fix package testing to use the live bits rather than the latest published bits. Will open an issue for that.

[dagood]

Also mention this? My first thought looking at this line is why it doesn't use patch version.

as for the patches and revisions should be manually updated per assembly if it is serviced

[safern]

Sure. will update the comment.

[dagood]

I'm concerned that this version is being used... the installer tests need to be running on the current build's outputs or else they aren't covering anything. 😕 Can you link the issue you mentioned you'd file for this?

[safern]

I'm surprised the installer tests use this property, I thought this property was used by package testing only.

I mentioned it here:
#1823 (comment)

Could you elaborate how the installer tests use this instead of the recently built product?

[dagood]

Could you elaborate how the installer tests use this instead of the recently built product?

No, I'm not familiar with it and I'm surprised by it when you mentioned you had to change this to make the installer tests work.

[safern]

I didn't mean installer tests, I mean libraries all configuration package tests... we run some tests on our oob packages to make sure closure is complete and that we don't ship duplicate types and that all supported rids for those packages publish correctly, etc.

[dagood]

Ah. This should be moved out from <!-- Installer dependencies --> then, it seems to me. Thanks for the background.

[safern]

I see, that was a result from the repo merge as in corefx it was "core-setup dependencies" maybe when we consolidated it was just a find and replace from "core-setup" to "installer". Will remove.

[safern]

Actually, what it means is that those dependencies as built by the installer. If you look the file, those comments split the sections on where are those packages coming from, for example look at runtime-assets dependencies which comes from https://github.com/dotnet/runtime-assets or arcade dependencies.

[dagood]

Oh, that makes sense. Sorry for the confusion on this, no need to change it.

[dagood]

I don't have enough context on the test expectations to help with the current CI error:

Assert.Equal() Failure
↓ (pos 20)
Expected: ComLibrary, Version=1.0.0.0, Culture=neutral, PublicKeyToken=···
Actual: ComLibrary, Version=5.0.0.0, Culture=neutral, PublicKeyToken=···
↑ (pos 20)
at Microsoft.NET.HostModel.ComHost.Tests.ClsidMapTests.PublicComVisibleTypeWithGuidAdded() in /_/src/installer/test/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.ComHost.Tests/ClsidMapTests.cs:line 35

Maybe @vitek-karas and @elinor-fung can help?

[safern]

Thanks @dagood -- I'm already looking at this locally, getting a repro and then see why ComLibrary is not getting the right AssemblyVersion.

[elinor-fung]

The test checks creating of a file (used for COM support) with data about a library - reading the assembly metadata; it uses the output assembly of the ComLibrary project (https://github.com/dotnet/runtime/tree/master/src/installer/test/Assets/TestProjects/ComLibrary) reading the assembly metadata.

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

[safern]

thanks for explaining @elinor-fung 😄

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

Me too, that''s why now I'm trying this locally to figure out why it didn't honor my AssemblyVersion in the props file for the test projects.

[safern]

This is now ready 😄

[dagood]

LGTM. (Can't approve "my" PR. 🙂)

[dagood]

Fixes: #1089

[safern]

Can we leave this set to true when we're in an Official build? So that package versions are unique?

[dagood]

Not necessary. https://github.com/dotnet/arcade/blob/bca68c2df2c6230389f10def3880a61db266c6c1/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L39

[safern]

Awesome, arcade is ahead of me 😄

[dagood]

Getting errors in Libraries build--a lot of these:

.packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.20063.2\build\Packaging.targets(1161,5): error : File Microsoft.Win32.Primitives, version 42.42.42.42 was included framework package Microsoft.Private.CoreFx.NETCoreApp/5.0.0-ci but that version is not considered inbox in package index F:\workspace_work\1\s\src\libraries\pkg\Microsoft.Private.PackageBaseline\packageIndex.json. Please add it with appropriate InboxOn entry for netcoreapp5.0 or suppress this message with PermitInbox suppression.

[safern]

Getting errors in Libraries build--a lot of these:

Yeah, the assembly version was affected by this change and now it is getting arcade's 42.42.42.42 version... we need to figure out how to set it up so that it respects our AssemblyVersions.

[dagood]

Might be tricky, getting Arcade to produce date-based assembly versions but not use the date for the packages. Maybe condition DotNetUseShippingVersions so it's specifically on during certain parts of the build? Don't know if that'll work out.

Feel free to push to this PR if you have time to work on it (or make a new one, no real difference).

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

[safern]

cc: @tmat@mmitche for discussion.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

Never mind I think we figured out the issue.

[dagood]

This matches https://github.com/dotnet/corefx/pull/41723/files#diff-8b8f08ffbf7b863fb3700c1718eeb4cbR5, we should maybe use ProductVersion or Major/Minor/PatchVersion though.

[safern]

cc: @ericstj I think it makes sense to use any of the recommendations above.

[tmat]

Yes, we should not have the version specified in multiple places.
I'd use <AssemblyVersion>$(MajorVersion).$(MinorVersion).$(PatchVersion).0</AssemblyVersion> if you want to suppress defaulting to 42.42.42.42.

[tmat]

I'd be also nice to add a comment that explains why is this set here - something like (guessing) "The AssemblyVersion must match netcoreappM.N moniker. Whenever the version is updated we need to update the TFM as well.".

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

To expand on this a bit (in case the assembly version fix doesn't work, or maybe something else breaks downstream), maybe we can keep DotNetUseShippingVersions as true, but set /p:_BuildNumber=$(BUILD_BUILDNUMBER) so that the version stays constant across the jobs.

[safern]

That might be a good idea, the problem I see with that is that it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

[safern]

It seems like the dependency to System.Private.CoreLib has the wrong version, I don't know if CoreLib also needs to be 5.0.0.0 or if we're calculating the dependency wrong. @ericstj might know.

C:\h\w\98B8088C\p\packageTest.targets(74,5): error : Assembly 'System.Runtime.Intrinsics.Experimental' has insufficient version for dependency 'System.Private.CoreLib' : 5.0.0.0 < 42.42.42.42. [C:\h\w\98B8088C\w\A0E70914\u\netcoreapp5.0\project.csproj]

[mmitche]

What's the context here? I think I'm missing what the root of the issue is.

[dagood]

See #1089

[tmat]

@dagood Are there any customizations to versioning in dotnet/runtime repo that tweak the default Arcade versions or are there any parts of the repo that use something different than the versions Arcade sets? I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement). The build should not depend on current time in any way. If there is some part of the build that depends on current date/time that'd be a bug.

[dagood]

I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement).

PR builds are not official builds, so we don't use OfficialBuildId. The Arcade docs include this guidance: Versioning.md#build-kind.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

[dagood]

As for why we have this issue:

it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

[tmat]

PR builds are not official builds, so we don't use OfficialBuildId.

I see. I mistaken this for official build issue. Got it. In PR validation though the version has Major.Minor.Path-ci format. No date based version number is added.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

I would rather avoid that. _BuildNumber is a private property not to be used outside of Arcade SDK targets.

[tmat]

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

Now I understand. Yes, don't set this.

[dagood]

You don't have to tell me. 😄

[safern]

I think we just need to figure out why we're getting the package test failures. I agree we shouldn't set DotNetUseShippingVersions. I'll take a look locally to see if I get a repro and a suggested fix.

[dagood]

Please feel free to push to this PR or make your own, I'm not actively working on this.

[safern]

@dagood I moved the AssemblyVersion declaration to Versions.props and also, I made it use $(MajorVersion).$(MinorVersion).0.0 -- as for the patches and revisions should be manually updated per assembly if it is serviced (talked to @ericstj on this).

Also, package testing is currently using a Microsoft.NETCore.App package from our custom feeds. I updated it manually to use the latest one. I didn't want to add a BAR subscription to the runtime repo because of the ongoing discussion regarding to that and we should eventually fix package testing to use the live bits rather than the latest published bits. Will open an issue for that.

[dagood]

Also mention this? My first thought looking at this line is why it doesn't use patch version.

as for the patches and revisions should be manually updated per assembly if it is serviced

[safern]

Sure. will update the comment.

[dagood]

I'm concerned that this version is being used... the installer tests need to be running on the current build's outputs or else they aren't covering anything. 😕 Can you link the issue you mentioned you'd file for this?

[safern]

I'm surprised the installer tests use this property, I thought this property was used by package testing only.

I mentioned it here:
#1823 (comment)

Could you elaborate how the installer tests use this instead of the recently built product?

[dagood]

Could you elaborate how the installer tests use this instead of the recently built product?

No, I'm not familiar with it and I'm surprised by it when you mentioned you had to change this to make the installer tests work.

[safern]

I didn't mean installer tests, I mean libraries all configuration package tests... we run some tests on our oob packages to make sure closure is complete and that we don't ship duplicate types and that all supported rids for those packages publish correctly, etc.

[dagood]

Ah. This should be moved out from <!-- Installer dependencies --> then, it seems to me. Thanks for the background.

[safern]

I see, that was a result from the repo merge as in corefx it was "core-setup dependencies" maybe when we consolidated it was just a find and replace from "core-setup" to "installer". Will remove.

[safern]

Actually, what it means is that those dependencies as built by the installer. If you look the file, those comments split the sections on where are those packages coming from, for example look at runtime-assets dependencies which comes from https://github.com/dotnet/runtime-assets or arcade dependencies.

[dagood]

Oh, that makes sense. Sorry for the confusion on this, no need to change it.

[dagood]

I don't have enough context on the test expectations to help with the current CI error:

Assert.Equal() Failure
↓ (pos 20)
Expected: ComLibrary, Version=1.0.0.0, Culture=neutral, PublicKeyToken=···
Actual: ComLibrary, Version=5.0.0.0, Culture=neutral, PublicKeyToken=···
↑ (pos 20)
at Microsoft.NET.HostModel.ComHost.Tests.ClsidMapTests.PublicComVisibleTypeWithGuidAdded() in /_/src/installer/test/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.ComHost.Tests/ClsidMapTests.cs:line 35

Maybe @vitek-karas and @elinor-fung can help?

[safern]

Thanks @dagood -- I'm already looking at this locally, getting a repro and then see why ComLibrary is not getting the right AssemblyVersion.

[elinor-fung]

The test checks creating of a file (used for COM support) with data about a library - reading the assembly metadata; it uses the output assembly of the ComLibrary project (https://github.com/dotnet/runtime/tree/master/src/installer/test/Assets/TestProjects/ComLibrary) reading the assembly metadata.

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

[safern]

thanks for explaining @elinor-fung 😄

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

Me too, that''s why now I'm trying this locally to figure out why it didn't honor my AssemblyVersion in the props file for the test projects.

[safern]

This is now ready 😄

[dagood]

LGTM. (Can't approve "my" PR. 🙂)

[dagood]

Fixes: #1089

[safern]

Can we leave this set to true when we're in an Official build? So that package versions are unique?

[dagood]

Not necessary. https://github.com/dotnet/arcade/blob/bca68c2df2c6230389f10def3880a61db266c6c1/src/Microsoft.DotNet.Arcade.Sdk/tools/Version.BeforeCommonTargets.targets#L39

[safern]

Awesome, arcade is ahead of me 😄

[dagood]

Getting errors in Libraries build--a lot of these:

.packages\microsoft.dotnet.build.tasks.packaging\5.0.0-beta.20063.2\build\Packaging.targets(1161,5): error : File Microsoft.Win32.Primitives, version 42.42.42.42 was included framework package Microsoft.Private.CoreFx.NETCoreApp/5.0.0-ci but that version is not considered inbox in package index F:\workspace_work\1\s\src\libraries\pkg\Microsoft.Private.PackageBaseline\packageIndex.json. Please add it with appropriate InboxOn entry for netcoreapp5.0 or suppress this message with PermitInbox suppression.

[safern]

Getting errors in Libraries build--a lot of these:

Yeah, the assembly version was affected by this change and now it is getting arcade's 42.42.42.42 version... we need to figure out how to set it up so that it respects our AssemblyVersions.

[dagood]

Might be tricky, getting Arcade to produce date-based assembly versions but not use the date for the packages. Maybe condition DotNetUseShippingVersions so it's specifically on during certain parts of the build? Don't know if that'll work out.

Feel free to push to this PR if you have time to work on it (or make a new one, no real difference).

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

[safern]

cc: @tmat@mmitche for discussion.

[safern]

We could also introduce an arcade property... something like UseAssemblyShippingVersions.

Never mind I think we figured out the issue.

[dagood]

This matches https://github.com/dotnet/corefx/pull/41723/files#diff-8b8f08ffbf7b863fb3700c1718eeb4cbR5, we should maybe use ProductVersion or Major/Minor/PatchVersion though.

[safern]

cc: @ericstj I think it makes sense to use any of the recommendations above.

[tmat]

Yes, we should not have the version specified in multiple places.
I'd use <AssemblyVersion>$(MajorVersion).$(MinorVersion).$(PatchVersion).0</AssemblyVersion> if you want to suppress defaulting to 42.42.42.42.

[tmat]

I'd be also nice to add a comment that explains why is this set here - something like (guessing) "The AssemblyVersion must match netcoreappM.N moniker. Whenever the version is updated we need to update the TFM as well.".

[dagood]

An alternative that fixes CI (but not dev build annoyances) is to pipe in a "non-official officialbuildid" so the date number stays constant throughout the course of the CI run.

To expand on this a bit (in case the assembly version fix doesn't work, or maybe something else breaks downstream), maybe we can keep DotNetUseShippingVersions as true, but set /p:_BuildNumber=$(BUILD_BUILDNUMBER) so that the version stays constant across the jobs.

[safern]

That might be a good idea, the problem I see with that is that it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

[safern]

It seems like the dependency to System.Private.CoreLib has the wrong version, I don't know if CoreLib also needs to be 5.0.0.0 or if we're calculating the dependency wrong. @ericstj might know.

C:\h\w\98B8088C\p\packageTest.targets(74,5): error : Assembly 'System.Runtime.Intrinsics.Experimental' has insufficient version for dependency 'System.Private.CoreLib' : 5.0.0.0 < 42.42.42.42. [C:\h\w\98B8088C\w\A0E70914\u\netcoreapp5.0\project.csproj]

[mmitche]

What's the context here? I think I'm missing what the root of the issue is.

[dagood]

See #1089

[tmat]

@dagood Are there any customizations to versioning in dotnet/runtime repo that tweak the default Arcade versions or are there any parts of the repo that use something different than the versions Arcade sets? I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement). The build should not depend on current time in any way. If there is some part of the build that depends on current date/time that'd be a bug.

[dagood]

I don't see how PR validation spanning a day boundary could be a problem if the whole build used $(OfficialBuildId) to derive version numbers (which happens to be a date based number, but that's not a requirement).

PR builds are not official builds, so we don't use OfficialBuildId. The Arcade docs include this guidance: Versioning.md#build-kind.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

[dagood]

As for why we have this issue:

it doesn't solve the local live live issue of people having to rebuild libraries/coreclr if they want to build again the installer a day after they built libraries or coreclr.

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

[tmat]

PR builds are not official builds, so we don't use OfficialBuildId.

I see. I mistaken this for official build issue. Got it. In PR validation though the version has Major.Minor.Path-ci format. No date based version number is added.

However, this is basically the same as my suggestion to set /p:_BuildNumber=$(BUILD_BUILDNUMBER).

I would rather avoid that. _BuildNumber is a private property not to be used outside of Arcade SDK targets.

[tmat]

It's because dotnet/runtime sets <DotNetUseShippingVersions>true</DotNetUseShippingVersions>.

Now I understand. Yes, don't set this.

[dagood]

You don't have to tell me. 😄

[safern]

I think we just need to figure out why we're getting the package test failures. I agree we shouldn't set DotNetUseShippingVersions. I'll take a look locally to see if I get a repro and a suggested fix.

[dagood]

Please feel free to push to this PR or make your own, I'm not actively working on this.

[safern]

@dagood I moved the AssemblyVersion declaration to Versions.props and also, I made it use $(MajorVersion).$(MinorVersion).0.0 -- as for the patches and revisions should be manually updated per assembly if it is serviced (talked to @ericstj on this).

Also, package testing is currently using a Microsoft.NETCore.App package from our custom feeds. I updated it manually to use the latest one. I didn't want to add a BAR subscription to the runtime repo because of the ongoing discussion regarding to that and we should eventually fix package testing to use the live bits rather than the latest published bits. Will open an issue for that.

[dagood]

Also mention this? My first thought looking at this line is why it doesn't use patch version.

as for the patches and revisions should be manually updated per assembly if it is serviced

[safern]

Sure. will update the comment.

[dagood]

I'm concerned that this version is being used... the installer tests need to be running on the current build's outputs or else they aren't covering anything. 😕 Can you link the issue you mentioned you'd file for this?

[safern]

I'm surprised the installer tests use this property, I thought this property was used by package testing only.

I mentioned it here:
#1823 (comment)

Could you elaborate how the installer tests use this instead of the recently built product?

[dagood]

Could you elaborate how the installer tests use this instead of the recently built product?

No, I'm not familiar with it and I'm surprised by it when you mentioned you had to change this to make the installer tests work.

[safern]

I didn't mean installer tests, I mean libraries all configuration package tests... we run some tests on our oob packages to make sure closure is complete and that we don't ship duplicate types and that all supported rids for those packages publish correctly, etc.

[dagood]

Ah. This should be moved out from <!-- Installer dependencies --> then, it seems to me. Thanks for the background.

[safern]

I see, that was a result from the repo merge as in corefx it was "core-setup dependencies" maybe when we consolidated it was just a find and replace from "core-setup" to "installer". Will remove.

[safern]

Actually, what it means is that those dependencies as built by the installer. If you look the file, those comments split the sections on where are those packages coming from, for example look at runtime-assets dependencies which comes from https://github.com/dotnet/runtime-assets or arcade dependencies.

[dagood]

Oh, that makes sense. Sorry for the confusion on this, no need to change it.

[dagood]

I don't have enough context on the test expectations to help with the current CI error:

Assert.Equal() Failure
↓ (pos 20)
Expected: ComLibrary, Version=1.0.0.0, Culture=neutral, PublicKeyToken=···
Actual: ComLibrary, Version=5.0.0.0, Culture=neutral, PublicKeyToken=···
↑ (pos 20)
at Microsoft.NET.HostModel.ComHost.Tests.ClsidMapTests.PublicComVisibleTypeWithGuidAdded() in /_/src/installer/test/Microsoft.NET.HostModel.Tests/Microsoft.NET.HostModel.ComHost.Tests/ClsidMapTests.cs:line 35

Maybe @vitek-karas and @elinor-fung can help?

[safern]

Thanks @dagood -- I'm already looking at this locally, getting a repro and then see why ComLibrary is not getting the right AssemblyVersion.

[elinor-fung]

The test checks creating of a file (used for COM support) with data about a library - reading the assembly metadata; it uses the output assembly of the ComLibrary project (https://github.com/dotnet/runtime/tree/master/src/installer/test/Assets/TestProjects/ComLibrary) reading the assembly metadata.

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

[safern]

thanks for explaining @elinor-fung 😄

It is hard-coded to expect version 1.0.0.0, but it looks like the output of the ComLibrary test project is now 5.0.0.0? I would expect that it should have been set to 1.0.0.0 based on this though: https://github.com/dotnet/runtime/pull/1835/files#diff-dcf139fe6da30186f3ea56c8ff1a086bR10?

Me too, that''s why now I'm trying this locally to figure out why it didn't honor my AssemblyVersion in the props file for the test projects.

[safern]

This is now ready 😄

[dagood]

LGTM. (Can't approve "my" PR. 🙂)