Skip to content

Disable ref pack build in 3.0 #14746

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 9 commits into from
Oct 14, 2019
Merged

Disable ref pack build in 3.0 #14746

merged 9 commits into from
Oct 14, 2019

Conversation

wtgodbe
Copy link
Member

@wtgodbe wtgodbe commented Oct 4, 2019

We should disable the build of the ref pack until the reference assembly versions are fixed (see #14538)

CC @dougbu @JunTaoLuo @nguerrera

JunTaoLuo
JunTaoLuo approved these changes Oct 4, 2019
View reviewed changes
Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

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

Yea I'm still working on some templating test fixes in the linked PR so let's make this change in case I don't get this in by tonight

dougbu
dougbu approved these changes Oct 5, 2019
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Small suggest might get this building…

@dougbu dougbu mentioned this pull request Oct 6, 2019
Merged
@JunTaoLuo
Copy link
Contributor

FYI you might need to apply the same fix as #14538 (comment)

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Oct 7, 2019
@wtgodbe
Copy link
Member Author

wtgodbe commented Oct 7, 2019

F:\workspace.1_work\1\s\src\Installers\Windows\Wix.targets(68,5): error MSB3030: Could not copy the file "F:\workspace.1_work\1\s\artifacts\bin\TargetingPack\x86\Release\AspNetCoreTargetingPack-x86.msi" because it was not found. [F:\workspace.1_work\1\s\src\Installers\Windows\TargetingPack\TargetingPack.wixproj]

error : '/home/vsts/work/1/s/artifacts/obj/TargetingPack.Layout/Release/' directory does not exist [/home/vsts/work/1/s/src/Installers/Debian/TargetingPack/Debian.TargetingPack.debproj]‌

And some test failures due to the ref pack not being present:

PlatformManifestListsAllFiles

System.IO.DirectoryNotFoundException : Could not find a part of the path 'F:\workspace\_work\1\s\artifacts\obj\TargetingPack.Layout\Release\packs\Microsoft.AspNetCore.App.Ref\3.0.1-ci\data\PlatformManifest.txt'.

Plus, all template tests fail because they depend on "restoring" the live-built ref-pack:

Project new reactredux failed to publish.\r\nProcess exited with code 1\nStdErr: \nStdOut: Microsoft (R) Build Engine version 16.3.0+0f4c62fea for .NET Core\r\nCopyright (C) Microsoft Corporation. All rights reserved.\r\n\r\nF:\workspace\_work\1\s\src\ProjectTemplates\test\bin\Release\netcoreapp3.0\TestTemplates\AspNet.reactredux.okac0s5wov4\AspNet.reactredux.okac0s5wov4.csproj : error NU1102: Unable to find package Microsoft.AspNetCore.App.Ref with version (= 3.0.1-ci)

Would it make more sense to disable publishing the ref pack, rather than disabling building it?

@JunTaoLuo
Copy link
Contributor

JunTaoLuo commented Oct 7, 2019 •
edited
Loading

Ideally it would make sense for us to not build the 3.0.1 ref packs and not refer to them in any way in tests. However, given the time crunch, it might make sense to build but not publish them. However, we should double check and make sure that the logic to refer to the 3.0.1-ci ref packs in our template tests is custom and users won't see this when using the official 3.0.1 bits when released.

@dougbu
Copy link
Contributor

dougbu commented Oct 7, 2019

Would it make more sense to disable publishing the ref pack, rather than disabling building it?

  1. The ref pack and targeting pack tests should also be disabled when not building the tested projects. Or, they should test the version of the packs specified in the baseline files.
  2. The templates should resolve the baseline version when the packs aren't being built. This likely means using the baseline version when $(IsTargetingPackBuilding) is false. Change would be in the ref and runtime projects because they are queried to get the right (often distinct) versions.

@JunTaoLuo
Copy link
Contributor

FYI @wtgodbe we need to update the logic at https://github.com/aspnet/AspNetCore/blob/release/3.0/eng/Versions.props#L29-L34. The baselines have been updated and this is a servicing branch now.

@wtgodbe
Copy link
Member Author

wtgodbe commented Oct 7, 2019

Testing a few fixes while I work out the best way to calculate the correct package version for the RefPack in the template tests

@JunTaoLuo
Copy link
Contributor

@wtgodbe you'll probably need my fix at 87f753a. nuget.exe pack doesn't use the trick. I suppose I could have removed the ItemGroup as a cleanup though.

dougbu
dougbu suggested changes Oct 8, 2019
View reviewed changes
Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

  1. Merge for 3.0.2 and not 3.0.1
  2. Up to you whether you fix IsServicingBuild (set DisableServicingFeatures to false) in this PR or we do that completely separately. If included, must remove PatchConfig.props and references to that file. The targeting packs and related installers are the only items not usually built in servicing builds for 3.0 and later.

src/Framework/test/TargetingPackTests.cs Outdated
@@ -56,6 +56,7 @@ public void AssembliesAreReferenceAssemblies()
}

[Fact]
[Skip]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
[Skip]
[Fact(Skip="link to an issue")]

src/Installers/Debian/Directory.Build.targets Outdated
@@ -49,7 +49,9 @@

<!-- Build SharedFx Bundle Deb package -->

<Exec Command="$(DebianBuildScript) -i '$(IntermediateOutputPath)' -o '$(IntermediateOutputPath)out/' -C '$(PackageContentRoot)'" />
<Exec
Condition="'$(IsTargetingPackBuilding)' != 'false'"
Copy link
Contributor

Choose a reason for hiding this comment

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

Likely minor but could put this Condition on the DebBuild target

@wtgodbe wtgodbe mentioned this pull request Oct 8, 2019
Closed
@wtgodbe wtgodbe force-pushed the DisableRefPackBuild branch from f51fa77 to b89103f Compare October 8, 2019 18:44
@dougbu dougbu mentioned this pull request Oct 11, 2019
Merged
@wtgodbe wtgodbe mentioned this pull request Oct 14, 2019
Merged
@wtgodbe wtgodbe merged commit 649ee1f into release/3.0 Oct 14, 2019
@wtgodbe wtgodbe deleted the DisableRefPackBuild branch October 14, 2019 18:23
@analogrelay analogrelay added this to the 3.0.1 milestone Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougbu dougbu dougbu requested changes

@JunTaoLuo JunTaoLuo JunTaoLuo approved these changes

@Vaseem88 Vaseem88 Vaseem88 approved these changes

@ryanbrandenburg ryanbrandenburg Awaiting requested review from ryanbrandenburg

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Milestone
3.0.1
Development

Successfully merging this pull request may close these issues.
6 participants
@wtgodbe @JunTaoLuo @dougbu @Vaseem88 @analogrelay @Pilchie