Skip to content

Bug#277#279

Closed
ymalich wants to merge 5 commits into
gluck:masterfrom
ymalich:Bug#277
Closed

Bug#277#279
ymalich wants to merge 5 commits into
gluck:masterfrom
ymalich:Bug#277

Conversation

@ymalich
Copy link
Copy Markdown

@ymalich ymalich commented Nov 6, 2020

Hello,
I'd fixed the problem found in the issue #277.
Would you like to review and merge it into the master?
My changes:

  1. Bugfix in the ResourcesRepackStep.
  2. New Integration Tests Scenarios to test the Bugfix:
    2.1 WindowsFormsTestNetCoreApp - new NET Core WindowsForms application with embedded resources.
    2.2 ClassLibraryCore - new NET Core class library with WPF user control using embedded resources .
    2.3. WPFSampleApplicationCore - new NET Core WPF using the user control from ClassLibraryCore.
  3. I've also fixed the new problem in integration tests - Nuget download doesn't work without SecurityProtocolType.Tls12 anymore. In order to use SecurityProtocolType.Tls12 the integration tests had to be bumped to net472.
  4. I've also added Nuget reference to NUnitTestAdapter to run tests in VS 2019.

Best regards,
Yury Malich

…orms and WPF).

- Fixed problem - Nuget download doesn't work without SecurityProtocolType.Tls12 anymore.
- Added NUnitTestAdapter package to run tests in VS 2019 TestExplorer.
@ymalich
Copy link
Copy Markdown
Author

ymalich commented Nov 6, 2020

"All checks have failed"

Error during project file parsing: Microsoft.Build.Exceptions.InvalidProjectFileException: The SDK 'Microsoft.NET.Sdk.WindowsDesktop'

Hm.. It seems Gradle must be updated to build new SDK-style NET Core projects.

@ngyukman
Copy link
Copy Markdown
Contributor

Hm.. It seems Gradle must be updated to build new SDK-style NET Core projects.

Hmm.. may be updating the com.ullink.msbuild to 3.8 can fix it

@ngyukman
Copy link
Copy Markdown
Contributor

I have fixed the build in #281

@ymalich
Copy link
Copy Markdown
Author

ymalich commented Feb 20, 2021

Great! Thanks!

@florian-gandon
Copy link
Copy Markdown
Contributor

florian-gandon commented Mar 2, 2021

Hi,

I've merged it from master. #284
It looks like two unit tests don't pass. I'll take a look but I recall GivenNetCore3WinFormsAppUsesImageResources_MergedCore3WinFormsApplicationRunsSuccessfully was failing prior to the merge. Maybe some ideas ?
Also, I've an issue with System.Resources.Extensions being repacked but at rutime it is still trying to load it so I've a FileNotFoundException. I'll take a look when I've time.

Regards,
Florian

@ymalich
Copy link
Copy Markdown
Author

ymalich commented Mar 2, 2021

Hi,

I've merged it from master. #284
It looks like two unit tests don't pass. I'll take a look but I recall GivenNetCore3WinFormsAppUsesImageResources_MergedCore3WinFormsApplicationRunsSuccessfully was failing prior to the merge. Maybe some ideas ?
Also, I've an issue with System.Resources.Extensions being repacked but at rutime it is still trying to load it so I've a FileNotFoundException. I'll take a look when I've time.

Regards,
Florian

Hi, Florian.
It was green on my side before merge.
I'm merging the latest master to my local branch right now, to fix the broken build/test.
I'll push my changes to update the pull-request then.

ymalich added 2 commits March 3, 2021 16:17
…neratesSameBamlAsSample.

The test is broken sometimes, because AssemblyInfoRecords were not sorted and the order in the list is not is not stable, it can change in some circumstances, but it does mean the Baml is wrong.
@ymalich
Copy link
Copy Markdown
Author

ymalich commented Mar 3, 2021

Hi,

I've merged it from master. #284
It looks like two unit tests don't pass. I'll take a look but I recall GivenNetCore3WinFormsAppUsesImageResources_MergedCore3WinFormsApplicationRunsSuccessfully was failing prior to the merge. Maybe some ideas ?
Also, I've an issue with System.Resources.Extensions being repacked but at rutime it is still trying to load it so I've a FileNotFoundException. I'll take a look when I've time.

Hi Florian,
I've merged the master-branch to my local branch, and fixed one blinking unit test.
All test are green right now on my side in Microsoft Visual Studio Community 2019 Version 16.8.6.

private List<BamlRecord> OrderAssemblyInfoRecordsByName(List<BamlRecord> bamlRecords)
{
var assemblyRecords = bamlRecords
.OfType<AssemblyInfoRecord>()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I check a bit the unit test failure so out of 19 records only than AssemblyInfoRecord are relevant. Good to know. :-)

Copy link
Copy Markdown
Author

@ymalich ymalich Mar 3, 2021

Choose a reason for hiding this comment

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

The test has failed sometimes on my laptop because BamlReader.ReadDocument returns AssemblyInfoRecords sorted in the different order: in one case the WindowsBase-assembly base goes first, in another case the PresentationCore.
I don't why, but the logical way to fix it is to sort the AssemblyInfoRecords.
I just put the sorted AssemblyInfoRecords at the end of the list, because the result is used only to compare the an another record list.

private List<BamlRecord> GetRelevantRecords(BamlDocument document)
{
return document.Where(node => !(node is LineNumberAndPositionRecord || node is LinePositionRecord)).ToList();
var list = document.Where(node => !(node is LineNumberAndPositionRecord || node is LinePositionRecord)).ToList();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Those checks could be merged with the AssemblyInfoRecord checks.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I'm not sure I understand your idea here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actually, all good, I missed the fact we put back the rest at line169.

@florian-gandon
Copy link
Copy Markdown
Contributor

I've fixed the other unit test. #286
I've VS 2019 as well but the pro. Maybe it depends on the msbuild installed.

@KirillOsenkov KirillOsenkov self-assigned this Dec 28, 2023
@KirillOsenkov
Copy link
Copy Markdown
Collaborator

Thank you all for the contributions. This has now been merged into master:

f808db7
01af4af

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants