Skip to content
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

Add Accessibility.dll to WinForms #593

Merged
merged 7 commits into from Mar 18, 2019

Conversation

Projects
None yet
4 participants
@ericstj
Copy link
Member

ericstj commented Mar 14, 2019

This is a primary interop assembly for OLEACC.dll.

It's needed because both Winforms and WPF expose public types that depend on the
COM interop types in OLEACC so they cannot embed the types.

@ericstj ericstj requested a review from dotnet/dotnet-winforms as a code owner Mar 14, 2019

Show resolved Hide resolved NuGet.Config
Show resolved Hide resolved Directory.Build.props Outdated
@ericstj

This comment has been minimized.

Copy link
Member Author

ericstj commented Mar 15, 2019

CI is failing because of this HelixPublish.proj ends up invoking the Build target on all the test projects and that rebuilds the product binaries. It appears to be rebuilding because configuration is not passed to the msbuild invocation: it ends up building debug in the release leg. It's also missing a number of other properties that are normally passed by build.cmd that could result in different results / failures. Here this is specifically failing because the binary build restored packages to a repo local .packages folder, but the HelixPublish.proj tries to use the user-profile packages folder (I believe because its missing the ContinuousIntegration property, but there could be others that are important) this leads to missing files and the failure.

@zsd4yr do you think you can take a look at fixing or disabling the HelixPublish.proj leg?

@zsd4yr zsd4yr referenced this pull request Mar 15, 2019

Merged

unblock issue with helix #596

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Mar 15, 2019

@zsd4yr do you think you can take a look at fixing or disabling the HelixPublish.proj leg?

For now I will revert to running tests on build machines to unblock your work here.

However, I am a little confused about why this is happening. I will start a thread with you, me, and @alexperovich to investigate

@zsd4yr

This comment has been minimized.

Copy link
Member

zsd4yr commented Mar 15, 2019

@ericstj you should be unblocked now

ericstj added some commits Mar 14, 2019

Add Accessibility.dll to WinForms
This is a primary interop assembly for OLEACC.dll.

It's needed because both Winforms and WPF expose public types that depend on the
COM interop types in OLEACC so they cannot embed the types.
Don't call ComputeIntermediateSatelliteAssemblies
Calling this target directly can cause it to run out of order.  This was
happening when the pkg project built before source, and was causing
CreateManifestResourceNames to remove all EmbeddedResources here
https://github.com/Microsoft/msbuild/blob/d42d3504057ef2b88dd4f68c4bfc5591371bd6fe/src/Tasks/Microsoft.CSharp.CurrentVersion.targets#L121
because we hadn't run the targets that set %(EmbeddedResource.Type).

Instead of doing this depend on the BuildOutputGroup.
Output groups are meant to be safe to call in isolation, so they should
always fully specify their target dependency chain to produce the
correct items without modifying the project state.
Update to latest IL SDK
And remove workaround for ILProj loading in VS.

@ericstj ericstj force-pushed the ericstj:Accessibility branch from 1305e4e to d7c7d9b Mar 15, 2019

@ericstj

This comment has been minimized.

Copy link
Member Author

ericstj commented Mar 15, 2019

Build is failing because we don't (can't) have a PDB. Need to suppress that check...

@ericstj

This comment has been minimized.

Copy link
Member Author

ericstj commented Mar 15, 2019

... and previous build succeeded because of MSBuild bug with aftertargets. dotnet/arcade#2267

Workaround Accessibility missing PDB
Missing PDB was resulting in a failure during builds when attempting to
push symbols.
@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 15, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@78ae911). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #593   +/-   ##
=========================================
  Coverage          ?   27.97%           
=========================================
  Files             ?      903           
  Lines             ?   243518           
  Branches          ?    32091           
=========================================
  Hits              ?    68136           
  Misses            ?   171562           
  Partials          ?     3820
Flag Coverage Δ
#Debug 27.97% <ø> (?)
#production 27.97% <ø> (?)
#test 27.97% <ø> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 78ae911...1823905. Read the comment docs.

@ericstj

This comment has been minimized.

Copy link
Member Author

ericstj commented Mar 16, 2019

Looks like this is passing now. Can get review / signoff / merge? Please let me know if you'd like any changes.

Show resolved Hide resolved NuGet.Config Outdated
@Tanya-Solyanik
Copy link
Member

Tanya-Solyanik left a comment

Please see minor comments about the line endings

@ericstj

This comment has been minimized.

Copy link
Member Author

ericstj commented Mar 18, 2019

It would be good to merge this before any of the darc PRs so that it doesn't hit a conflict on global.json/version files. /cc @zsd4yr @AdamYoblick @Shyam-Gupta

@zsd4yr zsd4yr merged commit 2fc563f into dotnet:master Mar 18, 2019

3 checks passed

codecov/patch Coverage not affected.
Details
codecov/project No report found to compare against
Details
license/cla All CLA requirements met.
Details
@ericstj

This comment has been minimized.

Copy link
Member Author

ericstj commented Mar 18, 2019

FYI folks, when we pick this up downstream we'll need to delete the old Accessibility package and use this instead. Feel free to ping me to help out on any of this. /cc @rladuca @vatsan-madhavan @stevenbrix @dagood

@vatsan-madhavan

This comment has been minimized.

Copy link
Member

vatsan-madhavan commented Mar 22, 2019

Is this going into the Winforms transport package, instead of a separate Accessiblity transport package?

Having a separate transport package would be better- we can then reference it from PresentationFramework, UIAutomationTypes and WindowsBase. Having to reference the full WinForms transport package from those projects just means that either we'll have to deal with potential side-effects due to namespace collisions, or have to filter out other references in a target prior to consumption. I can work with it either way, but would be nice to preserve existing design (separate trasnport package).

/cc @dotnet/wpf-developers

@ericstj

This comment has been minimized.

Copy link
Member Author

ericstj commented Mar 22, 2019

You don’t need a target. Reference with ExcludeAssets=All GeneratePathProperty=true then you can manually pull Accessibility from package using the property nuget writes to the obj/*props file. We don’t want Accessibility to have it’s own package since it isn’t shipping standalone. The individual packages was just a side effect of my initial prototype.

@ericstj

This comment has been minimized.

Copy link
Member Author

ericstj commented Mar 22, 2019

Here's a sample of what I am talking about:

  <ItemGroup>
    <PackageReference Include="Microsoft.Private.Winforms" Version="4.8.0-preview4.19171.1" ExcludeAssets="All" GeneratePathProperty="True" />
    <Reference Include="$(PkgMicrosoft_Private_Winforms)\lib\netcoreapp3.0\Accessibility.dll" />
  </ItemGroup>

This worked fine for me, let me know if it gives you any issues.

@vatsan-madhavan

This comment has been minimized.

Copy link
Member

vatsan-madhavan commented Mar 22, 2019

@ericstj Thanks - that should work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.