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

UpdateNugetPackages takes parameters and fixes common.nuspec requirements based on arch #4067

Conversation

mitchcapper
Copy link
Contributor

@mitchcapper mitchcapper commented Apr 16, 2022

Closes #4066

Summary:
The primary goal here is to only include the architectures the user wants to build in the required packages. There are also some code cleanup options for UpdateNugetPackages.ps1 to make it usable by more people. I debated if this should go in build.ps1 or UpdateNugetPackages.ps1 but it seemed more appropriate here (as the rest of the script is also doing version updates).

Changes:

  • Have the script take parameters rather than hard coded values, default to old hardcoded values if not specified
  • Add BuildArches parameter to the script like build.ps1, this is a requirement to know what final architectures the user wants to build for
  • Enabled script mode for the script, this required changing some of the node selection to xpath to avoid null child accesses
  • Use same nuget path and download function as build.ps1, I would assume no reason to use ../nuget
  • Major change: Edit the NuGet\CefSharp.Common.nuspec file to only require the cef dist files for the build users want. Rather than having an input file, or hardcoding the node data for the xml node we want added/removed we will just use the existing one as the template.
    - Added support for Async binding to return Task
    - Added new QUnit Test cases

How Has This Been Tested?
Yes tried setting different values and still seemed to update everything. I didn't see any files with EnsureNuGetPackageBuildImports targets any more but I believe that xpath should be correct.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • The formatting is consistent with the project (project supports .editorconfig)

@AppVeyorBot
Copy link

@amaitland
Copy link
Member

  • Use same nuget path and download function as build.ps1, I would assume no reason to use ../nuget

Your likely the first person to use UpdateNugetPackages.ps1, it was something I hacked together to make things easier when upgrading 😄 Reason for using a relative nuget.exe is that's what I always have on my machine. Having the script download if required makes sense 👍

  • Major change: Edit the NuGet\CefSharp.Common.nuspec file to only require the cef dist files for the build users want. Rather than having an input file, or hardcoding the node data for the xml node we want added/removed we will just use the existing one as the template.

This should have already been covered by build.ps1 as part of generating the packages. Is it not working?

I'd prefer to keep all changes to the nuspec files as part of build.ps1 if possible as it already has logic for dealing with the different architectures.

Do you need a specific target to use as part of your workflow? Happy to split that out as a new target if required

@mitchcapper
Copy link
Contributor Author

Mostly rather than using my own scripts before to change versions I figure if UpdateNugetPackages.ps1 can do it all the better. Plus as it already has to happen on new versions officially makes it a bit easier.

As for the build.ps1 instead for the feature change, no problem and will investigate why it did not work for me in current form. Last time I built with a custom version number and just x64 for the arches it still referenced the x86 redist package in the Common cefsharp package and failed to build.

As for why, there is an annoying windows 10 21H2 docker issue where they did not put out windows base images that match the kernel version. This forces you to use hyperV rather than straight process isolation and it takes forever. This is fixed in w11 but not currently looking to upgrade. Building only x64 cuts time way down.

@amaitland
Copy link
Member

Mostly rather than using my own scripts before to change versions I figure if UpdateNugetPackages.ps1 can do it all the better. Plus as it already has to happen on new versions officially makes it a bit easier.

Makes sense to use the same script if possible 👍 I'll merge the base changes later today so I can test when when I upgrade to M101 shortly.

As for the build.ps1 instead for the feature change, no problem and will investigate why it did not work for me in current form. Last time I built with a custom version number and just x64 for the arches it still referenced the x86 redist package in the Common cefsharp package and failed to build.

It's possible there's a mistake, happy to take a look when I get a minute.

As for why, there is an annoying windows 10 21H2 docker issue where they did not put out windows base images that match the kernel version. This forces you to use hyperV rather than straight process isolation and it takes forever. This is fixed in w11 but not currently looking to upgrade. Building only x64 cuts time way down.

Building only the arch you need sounds very reasonable to me 👍 I'm sticking with Win 10 as hardware I have is too old to run Win 11.

@amaitland
Copy link
Member

It's possible there's a mistake, happy to take a look when I get a minute.

Confirming there is a problem, I've forgotten to remove the dependency. Should be a simple fix.

@amaitland
Copy link
Member

I didn't see any files with EnsureNuGetPackageBuildImports targets any more but I believe that xpath should be correct.

I've tested locally and the EnsureNuGetPackageBuildImports aren't removed, adding the namespace to the xpath query got it working again.

@amaitland
Copy link
Member

Merged via cherry pick cfa5b41

Additional changes made in commit 78b682a

build.ps1 handles changing CefSharp.common.nuspec. Fixed the EnsureNuGetPackageBuildImports and did a little reordering of code so functions are first.

First commit to use the updated script is 684e76c

Everything looks OK so far.

Confirming there is a problem, I've forgotten to remove the dependency. Should be a simple fix.

The nuspec dep should be fixed in commit 80ea921

If you are still having problems with #4066 then please let me know.

@amaitland amaitland closed this Apr 20, 2022
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.

Building only select architectures does not have proper package requirements for .net framework.
3 participants