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.

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

Switch to Arcade targets for building VSIXes #29590

Merged
merged 24 commits into from Sep 12, 2018

Conversation

Projects
None yet
3 participants
@tmat
Member

tmat commented Aug 30, 2018

  • Replace vsmanproj and swxproj with standard SDK projects that use Arcade targets to produce the VSIX.

  • Replace handwritten SWR files with files generated from msbuild item groups and properties.

  • VSIXes that are uploaded to myget are unchanged (they are marked Experimental).

    Willow VSIXes to be inserted to VS are now built to Binaries\VSSetup\{config}\Insertion.

    The VSIXes have Experimental flag stripped and instead SystemComponent set:

    Before: <Installation Experimental="true">
    After: <Installation SystemComponent="true">

@tmat tmat requested review from dotnet/roslyn-compiler as code owners Aug 30, 2018

@tmat tmat changed the title from WIP: Arcade vsix to Switch to Arcade targets for building VSIXes Aug 30, 2018

@tmat

This comment has been minimized.

@tmat

This comment has been minimized.

Member

tmat commented Aug 30, 2018

/cc @agocke for compiler Willow packaging.

Build-InsertionItems
Build-Installer
}
}
# Not all of our artifacts needed for signing are included inside Roslyn.sln. Need to
# finish building these before we can run signing.
function Build-ExtraSignArtifacts() {

This comment has been minimized.

@jaredpar

jaredpar Aug 30, 2018

Member

Goodbye to that function!

$name = Split-Path -leaf $e
$filePath = Join-Path $configDir $e
$fullArg = "$baseArgs $filePath"
$vsixFiles = Get-ChildItem -Path $vsSetupDir -Include "*.vsix"

This comment has been minimized.

@jaredpar

jaredpar Aug 30, 2018

Member

Part of the reason we used an explicit array before was to guarantee ordering. This new code essentially has a non-deterministic ordering and I believe can lead to installation isuses because dependencies aren't guaranteed to be installed in the right order.

CC @jasonmalinoski

This comment has been minimized.

@tmat

tmat Aug 30, 2018

Member

I see. I'll revert and add a comment.

@@ -1,7 +1,7 @@
<?xml version="1.0" encoding="utf-8"?>
<PackageManifest Version="2.0.0" xmlns="http://schemas.microsoft.com/developer/vsx-schema/2011" xmlns:d="http://schemas.microsoft.com/developer/vsx-schema-design/2011">
<Metadata>
<Identity Id="7922692f-f018-45e7-8f3f-d3b7c0262841" Version="|%CurrentProject%;GetBuildVersion|" Language="en-US" Publisher="Microsoft" />
<Identity Id="7922692f-f018-45e7-8f3f-d3b7c0262841" Version="|%CurrentProject%;GetVsixVersion|" Language="en-US" Publisher="Microsoft" />

This comment has been minimized.

@jaredpar

jaredpar Aug 30, 2018

Member

CC @jmarlof as I believe he last had to deal with this.

@@ -5,19 +5,22 @@
<Platform Condition="'$(Platform)' == ''">AnyCPU</Platform>
<PlatformTarget>AnyCPU</PlatformTarget>
<OutputType>Library</OutputType>
<RoslynProjectType>Vsix</RoslynProjectType>
<RuntimeIdentifier>$(RoslynDesktopRuntimeIdentifier)</RuntimeIdentifier>

This comment has been minimized.

@jaredpar

jaredpar Aug 30, 2018

Member

Think we can delete this node now.

This comment has been minimized.

@tmat
<CompilerArtifact Include="$(_Exes)csc\net46\csc.exe.config"/>
<CompilerArtifact Include="$(_Exes)csc\net46\csc.rsp"/>
<CompilerArtifact Include="$(_Exes)csi\net46\csi.exe"/>
<CompilerArtifact Include="$(_Exes)csi\net46\csi.exe" NgenArchitecture="all" NgenApplication="csi.exe" NgenPriority="1"/>

This comment has been minimized.

@jaredpar

jaredpar Aug 30, 2018

Member

Why is there a priority entry on only some of the elements here?

This comment has been minimized.

@tmat

tmat Aug 30, 2018

Member

Don't know. We haven't had it on others. I preserved the values we used.

@@ -138,79 +124,6 @@ private bool CheckCoreClr(TextWriter textWriter)
dllRelativeNames);
}
private bool CheckPortableFacades(TextWriter textWriter)

This comment has been minimized.

@jaredpar

jaredpar Aug 30, 2018

Member

Where did this verification move to?

This comment has been minimized.

@tmat

tmat Aug 30, 2018

Member

It's correct by construction.

This comment has been minimized.

@tmat

tmat Aug 30, 2018

Member

I mean it doesn't seem to make sense to have two lists - one that lists what we want to include and the other that lists what we don't want to include and check that the first doesn't contain the latter.

This comment has been minimized.

@tmat

tmat Aug 30, 2018

Member

Besides, soon the list of facades will shrink to a few items.

This comment has been minimized.

@tmat

tmat Sep 7, 2018

Member

I mean to zero. We are not gonna need to build and insert PortableFacades.vsix.

@tmat

This comment has been minimized.

Member

tmat commented Sep 7, 2018

test windows_release_unit32_prtest please

@tmat

This comment has been minimized.

Member

tmat commented Sep 10, 2018

test this please

tmat added some commits Sep 10, 2018

@tmat

This comment has been minimized.

Member

tmat commented Sep 11, 2018

@jaredpar @jmarolf Any more feedback?

@tmat

This comment has been minimized.

Member

tmat commented Sep 11, 2018

test windows_debug_vs-integration_prtest please

@tmat tmat merged commit 515ff80 into dotnet:master Sep 12, 2018

16 of 17 checks passed

windows_debug_vs-integration_prtest Build finished.
Details
WIP ready for review
Details
license/cla All CLA requirements met.
Details
microbuild_prtest Build finished.
Details
roslyn-CI #20180911.23 succeeded
Details
ubuntu_16_debug_prtest Build finished.
Details
ubuntu_16_mono_debug_prtest Build finished.
Details
windows_build_correctness_prtest Build finished.
Details
windows_coreclr_debug_prtest Build finished.
Details
windows_coreclr_release_prtest Build finished.
Details
windows_debug_spanish_unit32_prtest Build finished.
Details
windows_debug_unit32_prtest Build finished.
Details
windows_debug_unit64_prtest Build finished.
Details
windows_determinism_prtest Build finished.
Details
windows_release_unit32_prtest Build finished.
Details
windows_release_unit64_prtest Build finished.
Details
windows_release_vs-integration_prtest Build finished.
Details

@tmat tmat deleted the tmat:ArcadeVSIX branch Sep 12, 2018

@tmat tmat restored the tmat:ArcadeVSIX branch Sep 12, 2018

333fred added a commit to 333fred/roslyn that referenced this pull request Sep 18, 2018

Merge remote-tracking branch 'dotnet/dev16.0.x' into merge-dev16.0.x
* dotnet/dev16.0.x: (512 commits)
  Sign CoreXT packages (dotnet#29986)
  Some more minor refactoring per feedback
  Address PR feedback and also handle few more cases identified during dogfooding: 1. Bail out for explicit interface implementations for properties, we only did so for methods. 2. Bail out for types with StructLayoutAttribute 3. Conservatively handle member references inside DebuggerDisplayAttribute argument by doing a simple string contains check.
  Change RemoveUnusedMembersAnalyzers to be a non-Style analyzer
  Fix issues identified during dogfooding.
  Create AddAwait refactoring (dotnet#28930)
  Address PR feedback (use static ctor and remove regions) and also fix an assert seen during dogfooding.
  Enable DeadCodeAnalysis rules and address design/review feedback.
  Disable VSTHRD010 to improve build performance
  Disable procdump since it prevents FailFast event logging
  Log information about crashes during integration tests
  Move xunit package references to Roslyn.Toolsets.Xunit.targets (dotnet#29664)
  Fixed a missed save
  PR feedback
  Fix race in SourceParameterSymbol
  Add option keywords to enable searchability (dotnet#29764)
  GenerateDeconstructMethod code fixer (dotnet#28286)
  Fix race in SourceEventSymbol.GetAttributesBag
  Switch to Arcade targets for building VSIXes (dotnet#29590)
  Add Make Field Readonly test for accessed fields
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment