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

Prevent error when installing more than one Chocolatey font package #455

Closed
KZeronimo opened this issue Oct 29, 2020 · 20 comments · Fixed by #479
Closed

Prevent error when installing more than one Chocolatey font package #455

KZeronimo opened this issue Oct 29, 2020 · 20 comments · Fixed by #479
Labels
5 - Released The issue has been resolved, and released to the public for consumption Bug Issues where something has happened which was not expected or intended Priority_LOW Issues that are low priority, and will most likely only be fixed by the community
Milestone

Comments

@KZeronimo
Copy link

KZeronimo commented Oct 29, 2020

What You Are Seeing?

Strange error when installing font choco packages via Boxtarter

What Is Expected?

Both choco packages install successfully which is what happens when executing outside of Boxstarter

How Did You Get This To Happen? (Steps to Reproduce)

Create a Boxstarter package with the following

cinst cascadia-code-nerd-font
cinst sourcecodepro

Output Log

edit: removed public gist of logs - key log output

ERROR: Cannot add type. The type name 'FontResource.AddRemoveFonts' already exists.
The install of cascadia-code-nerd-font was NOT successful.

@KZeronimo KZeronimo added the Bug Issues where something has happened which was not expected or intended label Oct 29, 2020
@flcdrg
Copy link
Member

flcdrg commented Oct 29, 2020

The install appears to be failing because it is trying to run extensions\New-FontResourceType.ps1 more than once (which has the call to Add-Type)

https://chocolatey.org/packages/chocolatey-font-helpers.extension mentions that there is a multiple font option, which https://chocolatey.org/packages/cascadia-code-nerd-font is not using.

However I just tested installing cascadia-code-nerd-font myself and can confirm it installs without error.

This is strange!

@pauby
Copy link
Member

pauby commented Oct 29, 2020

At a guess I'd say this is happening as Boxstarter runs those commands in that script in the same context. Boxstarter doesn't directly call choco so I'm assuming doesn't use it's PowerShell host that would isolate package installation. So when you're running the second cinst that resource is already there.

That's a guess. I'm not in a position to check just now. But I'd also think that this is a package issue.

A workaround would be to put a reboot between them?

@mwallner thoughts?

@mwallner
Copy link
Member

mwallner commented Oct 30, 2020

@flcdrg and @pauby both already got to the correct conclusion:

Since Boxstarter doesn't spawn a separate PowerShell host for each cinst the session scope is being shared, meaning all Add-Type calls may influence consecutive installations.

So both package installations add the Type FontResource.AddRemoveFonts. 😢

tbh, despite heavy Boxstarter usage this behavior never occurred in my environments, yet it's definitely something we haven't thought of.

as @pauby suggested, a forced reboot between the package installations may be used to bypass the error. (imo it's more a dirty hack than a workaround 😄)

could by 'fixed' by something like this:

cinst sourcecodepro

if (-Not (Test-Path "C:\rebootmarker")) {
  Get-Date | Out-File "C:\rebootmarker"
  Invoke-Reboot
}

cinst cascadia-code-nerd-font

I think the "installation scope separation" is something we should put on our agenda for the next Boxstarter release.

edit.: in addition to that, it may be a good idea to fix chocolatey-font-helpers.extension to be more fault-tolerant concerning loading of the FontResource methods.

@flcdrg
Copy link
Member

flcdrg commented Oct 30, 2020

A side comment to this: I happened to notice that the source code for the font extension package doesn't actually match what's in the latest version of the package. Might add a comment on the package for the maintainers

@KZeronimo
Copy link
Author

KZeronimo commented Oct 30, 2020

Hi @mwallner et al - thanks for looking into this - besides the mentioned "installation scope separation" topic for Boxstarter is there a recommendation for choco package authors/maintainers to be more defensive with Add-Type

edit.: true this - it may be a good idea to fix chocolatey-font-helpers.extension to be more fault-tolerant concerning loading of the FontResource methods.

I understand it's fine when running outside of Boxstarter but I think a lot of the choco usage is in the context of Boxstarter (at least for us)

FWIW automated font installation on Windows is harder than it should be - we have a pretty rich Boxstarter script to initialize our dev machines and the pain point is installing fonts?!?

Feel free to disposition this issue as you see fit

@pauby pauby added 0 - Backlog Issue is accepted, but is not ready to be worked on or not in current sprint Priority_LOW Issues that are low priority, and will most likely only be fixed by the community labels Oct 30, 2020
@pauby
Copy link
Member

pauby commented Oct 30, 2020

I really don't see a way around this at the moment. Looking at the help for Add-Type, under Notes it says:

Type names and namespaces must be unique within a session. You can't unload a type or change it. If you need
        to change the code for a type, you must change the name or start a new PowerShell session. Otherwise, the
        command fails.

So in the context of Boxstarter I'm not sure what we can do given the current design. We need to start a new session and the only way that can be done is via a reboot. Anything else would likely require major re-engineering of Boxstarter. If anybody disagrees please feel free to add here.

Long term this may be resolved by #394, if that goes ahead.

@flcdrg
Copy link
Member

flcdrg commented Oct 30, 2020

A possible mitigation might be to submit a patch to https://chocolatey.org/packages/chocolatey-font-helpers.extension so that the Add-Type logic can be run multiple times without error.

A followup to my comment on that package notes that @teknowledgist is now maintaining the source under https://github.com/teknowledgist/Chocolatey-packages/tree/master/extensions/chocolatey-font-helpers.extension

@KZeronimo
Copy link
Author

KZeronimo commented Oct 31, 2020

@flcdrg - I forked the chocolatey-font-helpers.extension from @bcurran3 (it was unclear exactly who was maintaining) and submitted a PR wrapping Add-Type with an existence check. I think that should fix the issue.

I suppose I can do the same for @teknowledgist implementation

BTW - when there are multiple maintainers of a choco package how do you tell which one is "shipping"

edit: @flcdrg - I saw your comments here - the contents of the package didn't match with the source linked on choco ...

@mwallner
Copy link
Member

ok ok ok ... I've cracked my head against this problem, and tbh, fixing the font packages is something we should do - but imo this is not a proper solution to the current problem @KZeronimo is facing.

I just came up with a crazy idea that might actually work and probably could be implemented in Boxstarter if we spend a little more thought on it! 😄

@KZeronimo try the following:
at the beginning of your boxstarter script, manually add the type that's required in the font packages.

   $fontCSharpCode = @'
   using System;
   using System.Collections.Generic;
   using System.Text;
   using System.IO;
   using System.Runtime.InteropServices;

   namespace FontResource {
       public class AddRemoveFonts {
           private static IntPtr HWND_BROADCAST = new IntPtr(0xffff);

           [DllImport("gdi32.dll")]
           static extern int AddFontResource(string lpFilename);

           [DllImport("gdi32.dll")]
           static extern int RemoveFontResource(string lpFileName);

           [return: MarshalAs(UnmanagedType.Bool)]
           [DllImport("user32.dll", SetLastError = true)]
           private static extern bool PostMessage(IntPtr hWnd, WM Msg, IntPtr wParam, IntPtr lParam);

           public enum WM : uint {
               FONTCHANGE = 0x001D
           }

           public static int AddFont(string fontFilePath) {
               FileInfo fontFile = new FileInfo(fontFilePath);
               if (!fontFile.Exists) { return 0; }
               try {
                   int retVal = AddFontResource(fontFilePath);
                   bool posted = PostMessage(HWND_BROADCAST, WM.FONTCHANGE, IntPtr.Zero, IntPtr.Zero);
                   return retVal;
               }
               catch { return 0; }
           }

           public static int RemoveFont(string fontFileName) {
               try {
                   int retVal = RemoveFontResource(fontFileName);
                   bool posted = PostMessage(HWND_BROADCAST, WM.FONTCHANGE, IntPtr.Zero, IntPtr.Zero);
                   return retVal;
               }
               catch { return 0; }
           }
       }
   }
'@
   Add-Type $fontCSharpCode

right after that, add a custom Add-Type function like this:

function Add-Type { Write-Host "yolo!" }

after that, do your

cinst somefont
cinst some_other_font
cinst whatever

at the very end of your boxstarter package, remove the custom added Add-Type function again:

Remove-Item function:Add-Type

ta-da 😄

@mwallner
Copy link
Member

@pauby @flcdrg see comment above, I think I might be onto something.
If we could rename the Add-Type function during boxstarter is active and wrap it in a "fault tolerant" equivalent which internally calls the Add-Type of module Microsoft.PowerShell.Utility we could fix this in a semi-clean manner which woulnd't requrie package maintainers to make any change.

@teknowledgist
Copy link

I just pushed v0.3 of the Chocolatey Font Helpers extension that should resolve the issue of trying to re-add the type and (hopefully) give better error messaging for an unusual font add/remove failure.

Let me know if it works (or not) for you.

@KZeronimo
Copy link
Author

KZeronimo commented Oct 31, 2020

@mwallner - edit: tried in Boxstarter context - removing public gist

Still getting ERROR: Cannot add type. The type name 'FontResource.AddRemoveFonts' already exists.

@teknowledgist will try the updated extension when it is approved (not sure how to test before that)

@mwallner
Copy link
Member

thanks for the feedback @KZeronimo! I'm not entirely sure why this doesn't work in Boxstarter context, yet seems to work just fine in a plain poweshell prompt. 😕

$fontCSharpCode = .. you know what ..

Add-Type $fontCSharpCode # actually adds the type

function Add-Type { Write-Host "yolo!" }
# cinst sourcecodepro
Add-Type $fontCSharpCode # prints yolo
# cinst cascadia-code-nerd-font
Add-Type $fontCSharpCode # prints yolo
# cinst notepadplusplus
Add-Type $fontCSharpCode # prints yolo

Remove-Item function:Add-Type

@flcdrg
Copy link
Member

flcdrg commented Oct 31, 2020

@KZeronimo the new 0.0.3 package is now approved, so give it a try.

I suspect installing the extension explicitly would also have worked. eg. add choco install chocolatey-font-helpers.extension --version=0.0.3 before your font packages.

@KZeronimo
Copy link
Author

KZeronimo commented Oct 31, 2020

@flcdrg - yep was trying that - I looked in the font packages I'm trying in this example - sourcecodepro and cascadia-code-nerd-font both don't seem to use the font extensions - it looks like they just include the boilerplate Add-Font and Remove-Font provided by MSFT which won't have the type existence check of the updated extension by @teknowledgist

The idea floated by @mwallner still might be useful for font packages not leveraging the choco font extension

@flcdrg
Copy link
Member

flcdrg commented Nov 1, 2020

@KZeronimo cascadia-code-nerd-font uses the extension, but yes unfortunately sourcecodepro doesn't - older font packages probably didn't know about it when they were created.

@KZeronimo
Copy link
Author

KZeronimo commented Nov 1, 2020

@flcdrg - do you see anything wrong with the test I'm trying to do?

edit: using plain old choc cinst sourcecodepro install successfully (not using extensions) cinst cascadia-code-nerd-font does install the updated chocolatey-font-helpers.extension 0.0.3 but fails the font install

Logs

edit: Remove gist of logs - key logs

The type or namespace name 'Win32Exception' could not be found (are you missing a using directive or an assembly reference?)

@teknowledgist
Copy link

Sorry! 🤦‍♂️ I think (hope!) it's fixed now with v0.0.3.1. cascadia-code-nerd-font is installing now at least.

(Anyone with C# error handling skills is welcome to contribute. 😄 )

@KZeronimo
Copy link
Author

@teknowledgist it looks like 0.0.3.1 is functioning as expected - I tested Install-ChocolateyFont and Uninstall-ChocolateyFont (single and multiple). I can also confirm the type existence check fixes the error reported here

The issue of legacy font packages that don't use the extension is a maintenance item

@flcdrg @mwallner - let me know if you want me to try anything else in the context of Boxstarter and the idea of the Add-Type intercept

@pauby pauby added this to the 3.0.0 milestone Oct 25, 2021
@gep13 gep13 added 3 - Review Code has been added, and is available for review as a pull request and removed 0 - Backlog Issue is accepted, but is not ready to be worked on or not in current sprint labels Apr 25, 2022
@gep13 gep13 added 4 - Done Code has been added to the repository, and has been reviewed by a team member and removed 3 - Review Code has been added, and is available for review as a pull request labels Apr 27, 2022
@gep13 gep13 changed the title Error when installing more than one choco font package Prevent error when installing more than one Chocolatey font package Apr 27, 2022
@gep13 gep13 modified the milestones: 3.0.0-beta-20220426-14, 3.0.0 Jul 14, 2022
@gep13 gep13 added 5 - Released The issue has been resolved, and released to the public for consumption and removed 4 - Done Code has been added to the repository, and has been reviewed by a team member labels Jul 14, 2022
@choco-bot
Copy link

🎉 This issue has been resolved in version 3.0.0 🎉

The release is available on:

Your GitReleaseManager bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Released The issue has been resolved, and released to the public for consumption Bug Issues where something has happened which was not expected or intended Priority_LOW Issues that are low priority, and will most likely only be fixed by the community
Projects
None yet
7 participants