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

[BUG] Use the buildTransitive folder #1041

Closed
mattleibow opened this issue Dec 2, 2019 · 19 comments · Fixed by #1057
Closed

[BUG] Use the buildTransitive folder #1041

mattleibow opened this issue Dec 2, 2019 · 19 comments · Fixed by #1057
Labels
area/Build area/HarfBuzzSharp Issues that relate to the C# binding of HarfBuzzSharp. area/SkiaSharp Issues that relate to the C# binding of SkiaSharp. type/bug type/enhancement
Milestone

Comments

@mattleibow
Copy link
Contributor

Make sure all projects with a .targets and/or .props file also use the buildTransitive folder.

https://github.com/NuGet/Home/wiki/Allow-package--authors-to-define-build-assets-transitive-behavior

@mattleibow
Copy link
Contributor Author

Just bringing in a comment from @vijayasurya20: #1050 (comment)

As you have mentioned, while installing the SkiaSharp into the test project works fine.

Since we have added the .Net Standard library project which has "SkiaSharp" as dependent package is added as dependency of Test project, then please let me know why should we need to add "SkiaSharp" package individually into the test project. Because the Library project itself should have every dependency it needs.

@mattleibow
Copy link
Contributor Author

@vijayasurya20 raises a good point - why should the test project need it to be specified. But this is not as straight forward as it may seem.

For .NET Core and all the platforms other than the full .NET Framework, the platform has the understanding that it should only reference the native libraries at the end of the chain. If I create a netcoreapp with a long dependency chain with only the last referencing SkiaSharp, the app still works. This is because the SkiaSharp reference is bubbled all the way up, and the final app project also includes the native binaries in the runtimes folder. As a result, everything works.

For the full .NET Framework, it does not have this idea of the runtimes folder, so I have to manually copy the files to the output directory. Right now, all this logic is in a build target. That build target is only referenced by the project that pulls in SkiaSharp. As a result, the MANAGED reference to SkiaSharp get bubbled up as it is a reference, but not the native binaries.

This is obviously not the best. But, if we make the targets file bubble up as well, then EACH project will have the native binaries in the output. Is this desirable?

We could make the native binaries only copy when the project is an app project (Exe or WinExe). This would work, but what about test projects (which is one of the reasons for this issue)?

After all this, we are at a place where .NET Core works fine and .NET Framework is a little dodgy. But, all we have to do is install SkiaSharp into the top-level project.

I am moving this to v1.68.2 until I can decide what the best thing to do is.

@dotMorten
Copy link

if we make the targets file bubble up as well, then EACH project will have the native binaries in the output

I used to limit this based on the output type (ie don't do it for class libraries). However that caused problems for people building plugins for other products, as they were now missing the native assemblies. So yes I'd say deploy it regardless.

@vijayasurya20
Copy link

Hi @mattleibow ,

I have checked in latest 1.68.1.1 release version, but still i am facing the same issue as raised in the following forum.
#1050

Please let me know whether it is fixed? If not so when can I expect the fix for this issue?

Regards,
Vijayasurya A

@mattleibow
Copy link
Contributor Author

I just tried your sample project attached to the other issue and upgraded. The tests ran in the IDE. All I did was unzip, open and upgrade.

@hafner
Copy link

hafner commented Jan 13, 2020

I just tried your sample project attached to the other issue and upgraded. The tests ran in the IDE. All I did was unzip, open, and upgrade.

So I'm having the same problem with my project. I also downloaded vijayasuya20's project, and it has the same error I'm getting. I unzipped, opened, and upgraded and I still get the same error. What can I try next? If we knew, it would help multiple people.

@vijayasurya20
Copy link

Hi @mattleibow ,

As @hafner said, I am also still facing the same error in my end.

Please let me know what should i need to do from my end to resolve this issue.

Regards,
Vijayasurya A

@hafner
Copy link

hafner commented Jan 15, 2020

What I don't get is both projects seem to have all of the dlls copied to the output directory yet it still doesn't work.

@TysonMN
Copy link
Contributor

TysonMN commented Jan 20, 2020

My experience is similar to @mattleibow's; his description below is also true for me.

I just tried your sample project attached to the other issue and upgraded. The tests ran in the IDE. All I did was unzip, open and upgrade.

In particular, instead of the test failing with a TypeInitializationException it now fails with a FileLoadException.

I poked around a bit further and figured the following out. Maybe it is helpful.

I have a simple WPF application with a dependency on the latest prerelease of SkiaSharp (version 1.68.2 (Preview Build 17)). The application works when targeting .NET Core 3.0 and throws a TypeInitializationException (c.f. #1050) when targeting .NET Framework 4.7.2.

I also found three workarounds:

  1. Change platform from "Any CPU" to "64-bit";
  2. Change platform from "Any CPU" to "32-bit"; and
  3. Leave platform as "Any CPU" but then copy netcoreapp3.0\runtimes\win-x64\native\libSkiaSharp.dll to net472.

In case three (when platform is set to "Any CPU"), it seems like the 32-bit version of libSkiaSharp.dll is copied into the output directory. In particular,

  • netcoreapp3.0\runtimes\win-x86\native\libSkiaSharp.dll and
  • net472\libSkiaSharp.dll

have the same MD5 hash.

@mattleibow
Copy link
Contributor Author

Ah, very good catch. That is often the case. The main issue here is that I think the test is built as 32-bit and then run as 64-bit (or vice versa). Switching to a specific architecture usually fixes that as @bender2k14 found. But, I believe you can ALSO switch the CPU that the tests run in in VS to control the runner.

@hafner
Copy link

hafner commented Jan 23, 2020

So my whole 4.7.2, ten project solution (including libraries and tests) were all targeting "Any CPU." After spending hours and hours on it what @bender2k14 said got me going, so thank you. I switched everything over to 32-bit. Although I need to research what the implications of doing this vs Any CPU are. Is an official fix coming?

@loketha
Copy link

loketha commented Jan 31, 2020

Hi @mattleibow

As per @bender2k14 suggestion, I checked the project with different configurations. But Still i am facing the same FileLoadException issue.

Please let me know what should i need to do from my end to resolve this issue.

Regards,
Loketha E

@mattleibow
Copy link
Contributor Author

mattleibow commented Feb 1, 2020

Does anyone have a repro project of this? Also, remember that MSBuild may detect that it needs to copy a 32/64 bit assemblies, and then VS will run the other platform. There is a option in the test explorer setting to pick what CPU arch to use.

Also, it might be the case that you have selected the "prefer 32-bit" in the project options. So, even though the runner is selecting 64-bit, the runtime actually thinks it is 32-bit.

What I usually do is make sure the "prefer 32-bit" is OFF in the project settings, build Any CPU and then make sure that my test explorer is running 64-bit. This way, MSBuild copies the 64-bit, and the runner is 64-bit.

Also note, the p[refer 32-bit option is just for Windows. I believe it is ignored on macos/linux. I might be wrong, but it is usually safe to disable.

@vijayasurya20
Copy link

Hi @mattleibow ,

First of all, Thank you for all your efforts.

As you have mentioned, we have checked our project and ensured that the "Prefer 32 bit" option is disabled both in our Library and also in the Test project. In both of our projects, we set the build target platform as "x86" only, but still, we are facing the same issue in our end. Can you please check this in your end using the project shared by us in the following link.

#1050

Please let us know if you have any questions.

Regards,
Vijayasurya A

@mattleibow
Copy link
Contributor Author

I just did that and it worked. Steps:

  • download zip
  • update SkiaSharp to v1.68.1.1
  • switch to x86
  • right click the project and select "Run Tests"
  • observe the green wave

I also notice this on both projects:

    <Platforms>AnyCPU;x86</Platforms>
    ....
    <PropertyGroup Condition=" '$(Configuration)' == 'Debug' ">
      <PlatformTarget>x86</PlatformTarget>
    </PropertyGroup>

You probably should not need that.

I removed them and the test was red. This is because the test runner now was using x86, but the compiler was optimizing for x64. To fix this, I just selected x64 from the test runner's settings:
image

But this last option is up to you.

@vijayasurya20
Copy link

Hi @mattleibow,

Thank you for your suggestion.

We have tried both of your suggestions in our end, but still, the persists in our end. For your reference, please find the video illustration below as follows.
Video.zip

Can you please check this and provide us solution?

Regards,
Vijayasurya A

@vijayasurya20
Copy link

Hi @mattleibow ,

Can you please share your suggestion on this as requested earlier.

Regards,
Vijayasurya A

@ManikandanRavichandranSync

Hi @mattleibow,

Can you please share your suggestion on this as requested earlier.

Regards,
Manish Ravichandran

@mattleibow
Copy link
Contributor Author

mattleibow commented Mar 12, 2020

@vijayasurya20 @ManishSync Sorry for the delay in getting back to you. I had a look at your screen capture and I think I see what is going on. All the features for supporting your scenario are not supported in VS2017. It is recommended to use VS2019.

The feature for buildTransitive folders in NuGets are only supported in NuGet v5. VS2017 only has NuGet v4.6.

Another issue that is sort of contributing to this not working is the fact that the office-processing is netstandard (which is a good thing). The .NET Standard projects do not make use of the .targets files that copy files because they make use of a new feature in NuGet - the runtimes folder. However, this is not coming into play right now because the test project is net471 - which does not support the runtimes feature.

There are a few solutions for VS2017:

  • Switch the office-processing project to net471 so that the .targets files start to come into play.
  • Multi-target the office-processing project so the tests can use net471 and the apps can use netstandard: <TargetFrameworks>netstandard2.0;net471</TargetFrameworks>.
  • Switch to netcoreapp2.2 for the tests: <TargetFramework>netcoreapp2.2</TargetFramework>. For this I did have to remove the platform things (all the x86). The .NET Core runtime will handle the native files
  • Install the SkiaSharp NuGet into the test project.

Let me know if any of these things help.

FoggyFinder added a commit to verybadcat/CSharpMath that referenced this issue Jun 21, 2021
… customizable (#164)

* Add TextColor property to BaseButton and make MathInputButton's color customizable

The main goal of this commit is to make keyboard colors customizable (per keyboard key) to fit an app's color theme(s). For example, a dark keyboard background with light button texts.

CSharpMath.Forms:
- Refactor BaseButton's constructor.
- Add TextColor property to BaseButton and a bindable TextColorProperty.
- Remove the predefined Red color from MathInputButton.InputToLaTeX(input) for "Backspace" and "Clear".

CSharpMath.Forms.Example (MathKeyboard.xaml):
- Use the TextColorProperty in the Style of "Backspace" and "Clear", setting it to Red.

* Add a few ButtonTests and refactor out use-once functions from BaseButton

Notes:
- I guessed that CSharpMath.Forms needs a Test project of its own.
- It seems to deviate from the current coding style to have such a class, but I needed to make sure that the unit test MathInputButtonsHaveBackTextColorByDefault could detect the "phantom" and thus I added a static class LatexHelper that contains the temporary implementation of a "fake" vphantom. I also added the method SetColor(latex,Xamarin.Forms.Color).

* Fix typo in unit test name MathInputButtonsHaveBlackTextColorByDefault

* ButtonTests: use [ClassData] attribute, use NotNull<T>() extension, include in slnf

- Instead of IEnumerable<MathInputButton> TheMathInputButtons, use the [ClassData] attribute, introducing TestHelper.ComplexClassData<T> that can pass classes instead of ValueType data.
- Instead of using the ! (null-forgiving) operator, introduce and use NotNull<T>() extension that throws a Xunit.Sdk.NotNullException if the object is null and otherwise returns the not-null object. (Note: the namespace deviates from the folder structure, so that it is available everywhere in the test project without the need to add a using statement.)
- Improve logic in Assert of AllMathInputButtonsHaveLatexContent.
- Add CSharpMath.Forms.Tests to CSharpMath.CrossPlatform.slnf.

Maybe the newly created classes can be used in other parts of the solution as well, but I don't feel comfortable deciding that. Most of the code is very compact and adding classes feels like ignoring that coding style. Also, I am not very confident that those classes I have created meet the expected standards. I therefore put them "close to where they are used".

* Update CSharpMath.CrossPlatform.slnf (suggested commit))

Co-authored-by: Hadrian Tang <[email protected]>

* ButtonTests: use the [MemberData] attribute and MathKeyboardInput enum for individual testcases + do some cleaning

- NotNull.cs: remove pragma directives and unused namespace usings.
- Rename LatexHelper's vphantom to phantom, as the intention is to also add a tiny bit of horizontal spacing.
- ButtonTests: use the [MemberData] attribute and remove TestHelpers.ComplexClassData. Use the MathKeyboardInput enum as method parameter type, since only xunit-serializable types will result in individual test cases.
- Remove unused linked file from CSharpMath.Forms.Tests.csproj.
- Restore deleted space from commit 4a56307, and remove another white line. (I committed the suggestion to see its result: I didn't understand why the space character seemed to be removed at the start of the line and I thought it had something to do with my Visual Studio settings. Now I know that the suggested change was removing the white line and that the removed space character was not the intention.)

* Rename and move file from TestHelpers/NotNull.cs to /Extensions.cs

* Add ButtonBase.TextColorProperty Xaml Tests to CSharpMath.Forms.Tests

Notes:
- I changed the namespace of Extensions.NotNull and made the class partial, so that it can be linked from any test project.
- I am still in doubt about what is the right folder/project structure for the tests.

Long description of this last comment:
-- The name Xaml refers to the markup language and if code behind stuff should be included in that project, then maybe "UI" is a better name than "Xaml".
-- The class names are very broad: for example, "Test" seems to say that that is the main file that should include all tests. But that is set up only for tests that are shared between Avalonia and Xamarin.Froms. When thinking about creating a file that is for testing Xamarin.Forms only, I bump into the fact that one file is called TestXamarinForms.cs but that file has a completely different purpose than being the container for unit tests.
-- Postponing the decision to restructure - or actually awaiting to hear your preferences - I used external links. I wouldn't mind making the change or even thinking up a new the structure, but without that request I won't do that of course (as a newbie/guest to the project).

* Add MathInputButton_Command unit test and do the suggested cleaning

- Add MathInputButton_Command unit test.
- The global XML Namespace is only needed for the outermost node.
- Move a refactored NotNullExtension into ButtonTests.cs.

* MathInputButton_Command test: use the MathInputButton's Keyboard getter

* MathInputButton_Command test: no variable is needed anymore for the MathKeyboard instance

* Revert "MathInputButton_Command test: no variable is needed anymore for the MathKeyboard instance"

This reverts commit 132e19f.

* Revert "MathInputButton_Command test: use the MathInputButton's Keyboard getter"

This reverts commit fe0f460.

* Add MathButton unit tests that check that the image color is correctly set in the StreamImageSource

Probably needless to say: if ever the font changes, new images can created by adding the following method temporarily to the unit test and copying the result into the test project's "files/buttons" folder:
static void StreamToFile(Stream s, FileInfo f) { using (var fs = f.Create()) { s.CopyTo(fs); }}

* Attempt/test 1: also reference SkiaSharp from the CSharpMath.Forms.csproj

* Attempt/test 2: also reference SkiaSharp from CSharpMath.Forms.Test.csproj

* Attempt/test 3: Reference SkiaSharp & SkiaSharp.Views.Forms from test project (note: my previous commit message stated that I included SkiaSharp, but it was SkiaSharp.Views.Forms)

* Revert "Attempt/test 3: Reference SkiaSharp & SkiaSharp.Views.Forms from test project (note: my previous commit message stated that I included SkiaSharp, but it was SkiaSharp.Views.Forms)"

This reverts commit b43a974.

* Revert "Attempt/test 2: also reference SkiaSharp from CSharpMath.Forms.Test.csproj"

This reverts commit 350fc9b.

* Revert "Attempt/test 1: also reference SkiaSharp from the CSharpMath.Forms.csproj"

This reverts commit 8c486dd.

* Update SkiaSharp and SkiaSharp.View.Forms to 2.80.2 in all referencing projects

Introduces unwanted side effects:
- Warning NU1605 Detected package downgrade: Xamarin.Forms from 4.6.0.772 to 4.3.0.908675.
- A reference to a dll from CSharpMath.Forms.Tests, because somehow referencing the NuGet package wasn't enough. This seems to be a known bug: mono/SkiaSharp#1393.

* Add libSkiaSharp.dll to test project CSharpMath.Forms.Tests

Trying out a variation of workaround 3 from:
mono/SkiaSharp#1041 (comment)

* Revert "Add libSkiaSharp.dll to test project CSharpMath.Forms.Tests"

This reverts commit 593d677.

* Revert "Update SkiaSharp and SkiaSharp.View.Forms to 2.80.2 in all referencing projects"

This reverts commit 31cc12a.

* Customize buttons for MathKeyboardInput even more

- Make the button display Latex related to the enum values of MathKeyboardInput overridable. (Not the math that may be displayed after pressing the button, but only the appearance of the button itself.)
- Use a TextButton as a base class for MathInputButton. Then you have more options than when using a MathButton. (See the Example project, MyMathInputButton.cs where just the text "space" is used.)
- Add another button type related to MathKeyboardInput (but unrelated to LaTeX): ImageSourceMathInputButton. This may help people be creative. (See the Example project, the Clear button now appears as a flame: Controls/ImageSourceMathInputButtons/flame.png.)
- Add a unit tests:
--- ImageSourceMathInputButton_InputProperty_KeyboardProperty_and_Command
--- MathInputButton_KeyboardProperty

* Make blinking Placeholder's Nucleus and ForeColor customizable in both CaretStates (#167)

* Make blinking Placeholder's Nucleus and ForeColor customizable in both CaretStates

* Renaming of Placeholder-related variables + refactor (#167)

- Rename Placholder's "ForeColor" to "Color".
- Use property initializer GlyphInfo.Foreground.
- Restore "readonly field" instead of property getter for LaTeXSettings.Dummy.
- Use the name parts "Resting" and "Active" in the placeholder setting names (instead of "Hiding" and "FullShow" which are related to the caret but do not fit a blinking placeholder).
- Use variable name "placeholder" instead of "ph".

* Add unit tests for customizable placeholder (#167)

* Disable parallelization of customizable placeholder unit tests

Also: verify more in LaTeXSettings_Placeholder_IsNewInstance.

* Add unit test AllCustomizablePlaceholderPropertiesAreResetOnCaretVisible (#167)

Also: in the MockTests class, verify that AttributedGlyphRun sets the GlyphInfo.Foreground to null (default color).

* Unit test CustomizedPlaceholderBlinks: test complete cycle

* Fix failing unit test CaretTimerResetsOnKeyPress

* Use Assert.All instead of Assert.True(enumerable.All(pred))

* Revert "Fix failing unit test CaretTimerResetsOnKeyPress"

This reverts commit 9925952.

* Replace Assert.NotEqual + replace hardcoded strings by constants

* Refactoring and cleaning (of customizable placeholder tests and more)

* Placeholder tests: use Assert.NotSame and async Task

* MathInputButtons with customized placeholders: what you see is what you get in the output

In case MathInputButtons have the same TextColor as the output, the placeholders can be customized by only setting one or more of the LaTeXSetting properties PlaceholderActiveNucleus, PlaceholderRestingNucleus, PlaceholderActiveColor, PlaceholderRestingColor.
In case of different keyboard colors than output colors then the MathInputButton properties PlaceholderActiveColor and/or PlaceholderRestingColor can be used to override the LaTeXSettings. Those two properties use a weird hack: BindableProperties cannot have nullable value types it seems. Nobody will ever use a transparent placeholder, so that is save to use as a replacement for null. The color black should be available, in case the TextColor is not black.

This commit reverts the adding of MyMathInputButton of commit 94ebda2.

The Example project's Editor tab is updated to have a "Change appearance" button on the EditorPage that does a round trip through 3 themes that show different Clear buttons, different colors on the keyboard keys and output, different placeholders on the keyboard and in the output.

A large number of testcases has been added. It does not cover everything however.
The PlaceholderColorsProperties_MathInputButton expects a weird LaTeX string (you see "{}" at index 24):
@"\(\color{blue}{\square }{}^{\color{green}{■}}\)"

* Correct comment

* MathInputButton: only perform the placeholder color logic if defaults are overridden + refactor

- At ButtonDraw: only perform the placeholder color logic if the placeholder properties (of LaTeXSettings or of MathInputButton) are not the default values (null).
- Introduce NullableColorBindablePropertyHelper for nullable color workaround.

* Comment out 2 ButtonTests that have Linux failures

* Use FactAttribute.Skip

* Delete NullableColorBindablePropertyHelper (I am afraid I did not try the right things before - it just works)

* Revert "Delete NullableColorBindablePropertyHelper (I am afraid I did not try the right things before - it just works)"

This reverts commit da8b291.

* Use "foreach" instead of Xamarin.Forms.Internals.ForEach

* First call SetButtonsTextColor and then SetClearButtonImageSource

* Update CSharpMath.Forms.Example/CSharpMath.Forms.Example/Controls/MathKeyboard.xaml.cs

Co-authored-by: FoggyFinder <[email protected]>

* Add unit tests MathButtonTextColorCanChangeMultipleTimes and MathInputButtonTextColorCanChangeMultipleTimes

Also:
- Skip only on Linux via "FactSkipLinux(reason)" instead of a general Fact(Skip = reason).
- Move some helper methods into a separate file ButtonTestsHelper.cs.
- Rename png used in unit tests, using more specific names.

* Introduce interface IButtonDraw for bindablePropertyChanged

- Move ButtonDraw from TextColor setter to TextColorProperty's propertyChanged event.
- If calling ButtonDraw after casting to a base class, the subclass' override of ButtonDraw() is not executed, but via the IButtonDraw interface it is.
- Move NullableColorBindablePropertyHelper to its own file.
- ButtonTestsHelper refactor: extract method imageButton.ImageSourceAsStream().

Example project:
- use PlaceholderBlinks setting in the thrid theme.
- call ButtonDraw during each theme change (this is officially only needed for going from theme 1 to theme 2, but may also fix a not-understood bug).

* Shut CSharpMath.Ios.Tests

* add lock for now

* Create and use method SubStringCount() instead of Regex.Matches().Count

* Customized placeholder colors: add unit test

Co-authored-by: Hadrian Tang <[email protected]>
Co-authored-by: FoggyFinder <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/Build area/HarfBuzzSharp Issues that relate to the C# binding of HarfBuzzSharp. area/SkiaSharp Issues that relate to the C# binding of SkiaSharp. type/bug type/enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants