Repository navigation
Conversation
The integration scenarios build their configuration directly rather than through the ConfigurationBuilder, so any nested configuration property a constructor does not initialise stays null. Shim generation reflects over the whole configuration and recurses into every non-builtin property, so a null one throws "Non-static method requires a target", the install is quarantined to lib-bad, and the error names neither the property nor the builder. This bit the Chocolatey Licensed Extension harness, where every install of a package containing an .exe failed. Add a spec that walks the baseline configuration and fails, naming the property, if a nested property is null, so the next one added without a default is caught at the cause.
A package declares that it may be installed more than once, as named instances, with the supportsInstances nuspec field. The Chocolatey fork of NuGet.Client now reads that field, but nothing on the CLI side exposed it, so an extension had no way to ask whether a package allows instances. Parse it onto ChocolateyPackageMetadata in all three constructors, the same way the other Chocolatey-specific nuspec fields are, treating a missing or unrecognised value as false. The CLI itself never acts on it; it is there for the licensed extension to consume.
A package instance cannot share the lib\<id> folder a default install uses, and two instances of one package cannot share a folder either. The path resolver only knew the single lib\<id> layout. Give ChocolateyPackagePathResolver an optional instance name. Without one, paths are unchanged. With one, the requested package resolves to <root>\<id>\<instance>. When a parent id and dependencies root are also set, every other package in the install is a dependency of the parent and resolves to <dependenciesRoot>\<id>, the ordinary layout, so a dependency is never installed as an instance and is shared with everything else that needs it. The existing constructor is kept and delegates to the new one. Specs cover the default layout, the instance layout, and parent and dependency routing, including case-insensitive parent matching.
Exporting installed package instances and installing them again from the resulting packages.config needs each entry to say which instance it is. The packages.config model had no attribute for that, so the value could not be written or read back. Add an optional instance attribute to PackagesConfigFilePackageSetting. An entry without it deserialises exactly as before. The existing specs only mock the deserialiser, so nothing exercised the XML mapping itself. Add specs that do: a fully specified entry, an entry with an instance, an id-only entry and its defaults, multiple entries, and a serialise/deserialise round trip.
A package instance keeps its own information store, so both the result of an operation and its stored information need to say which instance they belong to. Neither type could, and the information service always derived its store path from the package id and version alone. Add Instance to PackageResult and ChocolateyPackageInformation. Route the information service's reads, saves and removals through a protected Load, a protected virtual Remove, and a protected virtual GetStorePath taking the package information, so an extension can locate an instance's store without duplicating the reading logic. The default path is unchanged. Add IInstanceAwarePackageInformationService for the instance overloads rather than extending IChocolateyPackageInformationService, so existing implementers of that interface are not broken.
A package that supports instances needs to know, from inside its own scripts, which instance it is being installed, upgraded or uninstalled as, so it can keep per-instance state such as a service name or a data folder apart. The PowerShell service sets a fixed list of package environment variables and offered no way to add to it. Add a protected virtual SetAdditionalPackageEnvironmentVariables, called after the standard package variables are set and before the per-run variables are cleared. The default does nothing. Also add the ChocolateyPackageInstance environment variable name, which the licensed extension sets through this hook.
The licensed extension implements package instances by changing where a package is installed, whether it may be installed, how it is shimmed, where its information is kept, what export writes, what a package is called in the output, and what an install, upgrade or uninstall must not break. All of that sat inside private or non-virtual members of NugetService, ChocolateyPackageService and ChocolateyExportCommand, so the only way to change it would have been to copy those methods whole. Add protected virtual hooks at each of those points, every one of which defaults to the current behaviour: - Install: the path resolver, the packages location, preparing the resolver per requested package, skipping a requested package, refusing an extracted package (removing its files again), and annotating a result once its files are in place. - Install, upgrade and uninstall: the dependencies of packages installed outside lib. They are given to the dependency resolver as constraints and are never installed or removed by it, so a change that they do not accept is refused, as it is for a package under lib. The uninstall was not given them at first, so uninstalling a package that an instance depends on removed it from under the instance, where uninstalling a package that another package under lib depends on is refused. It is refused with the same message now, and forcing it works as it does for packages under lib. - Uninstall: the message for a package that is not installed, plus virtual GetInstalledPackages. - ChocolateyPackageService: installing and removing shims, removing package information, the key a result is collected under, extra packages.config settings, and the bad-package path handling. - Export: the packages to export and extra attributes on each entry, with the command's members made virtual. - The name of a package in the output. Install, upgrade and uninstall name each package by its id, so when an extension installs the same package more than once, as separate instances, every line for those installs reads identically: the install header, the package files and success lines, and the end-of-run lists of installed packages, warnings, reboots and failures. Add a protected virtual GetPackageDisplayName to NugetService and ChocolateyPackageService, defaulting to the package id, and route those display uses through it. Paths, locks and result keys still use the id. The install header is now logged after OnPackageExtracted rather than before the result exists, so an extension's annotations on the result are available when the name is built. The default output is unchanged. RemovePackageFromCache and ValidatePackageHash become protected so an extension installing to its own location can reuse them. Add IInstanceAwareNugetService for enumerating installed instances rather than extending INugetService, so existing implementers are not broken, and the include-instances option name and description.
The remembered arguments of a package are read from the default .chocolatey\<id>.<version> store, and both ArgumentsUtility and the upgrade built that path themselves, so a package that keeps its information somewhere else cannot have them shown or restored. The licensed extension keeps the information of each package instance in its own store, so listing instances with --verbose could not show their remembered arguments, and the upgrade of an instance could not restore them. Add an overload of DecryptPackageArgumentsFile that takes the path of the arguments file. The existing method builds the default path with a new public GetPackageArgumentsFile, so it is written once, and delegates to the overload, so its behaviour is unchanged. Read the path for the upgrade from a protected virtual method of NugetService, GetRememberedArgumentsFile, that returns the file in the default store unless it is overridden. An extension that keeps a package's information elsewhere can then point SetConfigFromRememberedArguments at its own store and reuse the rest of it: the check of the feature, the warning for unbalanced quotes, the parsing, and keeping the source credentials the user gave on the command line. Nothing changes for a package in the default store. Specs cover reading from a given path, a missing file, redaction of sensitive values, and that the default path is never built. Other specs use a service that overrides the method: the service asks it about the package it upgrades, without an override it reads the arguments file of the package in the default store, and with an override it reads the file it returned and no other.
choco list only lists locally installed packages, so --page and --page-size have nothing to page: every result is already returned. They matter even less now that package instances are listed from the local machine, where paging them is not supported. Keep accepting the options so existing scripts do not break, but ignore them and warn that each one is obsolete and that all results are returned. The warning goes to the log file only when --limit-output is used, so machine-readable output stays clean. Specs cover the warning for each option, with and without --limit-output, that a command that uses neither warns about nothing, that one that uses only one warns about only that one, that a value that is not a number is ignored without an error where it used to be an error, and that both options are still accepted and described as obsolete so that scripts that pass them do not break. Warning about both options whichever is used was the change that the first specs could not see. Removing the warnings, making a value that is not a number an error, taking OBSOLETE out of either description and no longer accepting the page size option each fail specs for them.
The licensed extension changes how packages are installed, shown and stored by overriding protected members and by reading new metadata, and nothing in this repository exercised those points, so only the extension's own tests would notice a change to how they are used. Add specs and scenarios that use each of them the way an extension does, with a service or command that records its calls and can be told what to return, and with every default left as it was. Packages outside lib. The hook that reports their dependencies is used by the install, upgrade and uninstall resolutions. With a service that overrides it, the default reports no packages, and a hook that returns nothing is handled as if there were none. Uninstalling a package that a package outside lib needs is refused and says which package depends on it, and is allowed with --force. Installing a package that needs another version of a dependency than one outside lib accepts is refused and changes nothing, where the same install succeeds when nothing outside lib needs the dependency. Upgrading a package whose new version needs another version of a dependency changes the dependency when nothing outside lib needs it, and is refused when one accepts only the installed version. The refusals are checked as an error that names the package outside lib and the version it accepts, because without it both a refusal and a resolution that never heard of the package outside lib leave everything as it was. Install and uninstall members. The install and the uninstall call the members that choose where a package is installed, whether it is, whether it is kept once extracted, how it is shown, and what is said when an uninstall targets a package that is not installed. With nothing overridden the package is installed under lib and the members are called in the order the install reaches them. A package that is skipped is not extracted, and one that is refused after it is extracted is removed, reported as an error and failed, without being annotated or run. The name given is the one shown, and the result keeps the id. A service that reports another resolver and location installs there, and one that reports only another location has the folder created. Package service members. Handling the result of an install reads the information of the package, creates its shims and saves the information, in that order, and does not when the service takes the work over. Handling an uninstall removes the shims and then the information, only removes the information when the feature for it is on, and leaves either to the service when it takes them over. A packages.config that lists a package twice collects one result by default and one for each when the service tells the results apart, and the service is asked about each entry and adds to it after the standard settings. The name the service gives is the one used in the output while a result is handled, when the install is reported as successful, and in the messages for a failed install, a reboot after an install and after an uninstall, a failed uninstall, and the summaries of an install from a packages.config. The two lines that say a package can be uninstalled automatically are not covered, as they need the registry and a real install. Export. With nothing overridden every package the service has installed is exported with only its id, and its version after that when versions are included. A command that chooses the packages exports those and does not ask the service, and one that adds an attribute has it written after the id and before the version of each package. Script environment. A service that adds nothing sets the name and the folder of the package and none of the variables an extension would add. A service that adds variables is asked once, with the configuration and the folder of the package, after the name and the folder are set and before the variables that are not passed on to the next package are cleared. Preparing the environment for no package does not ask the service. The specs put the process environment back afterwards. Metadata and stores. The metadata of a package includes whether its nuspec declares supportsInstances, parsed in three places, one for each way the metadata is read: a nuspec reader, a nuspec file and a nupkg file. For each, true, True, TRUE, a value with spaces and 1 read as true, and an absent element, false, 0, an empty element and a value that is not a boolean read as false. The package information service locates its folder through one overridable method, and its specs cover the current store, the current store winning over a legacy one, the legacy store named with four version numbers or with two, and that reading, saving and removing each follow a service that reports another location and do not touch the default one. The path resolver specs cover a parent id with no dependencies root or only a blank one, an instance name that is only whitespace, a parent id on a resolver with no instance, the file name of an instanced package, resolving by identity giving the path that resolving by id does, and the parent id changing between packages. Each spec was checked by breaking the line it is meant to pin: removing the call of a member, using the id where the name should be, ignoring the reported location, and the other changes in each area each fail the specs for them. The parent id changing between packages has nothing to break, as the resolver reads it on every call, so that spec records the behaviour only.
Chocolatey.NuGet.Client 3.6.0 adds the supportsInstances nuspec metadata field, which Package Instances reads to decide whether a package can be installed as an instance. It also adds the IChocolateyPackageSearchMetadata property and removes the hard coded starts-with entry. Bump the package reference from 3.5.0, and update the entry in CREDITS.json to match, with the license link pinned to the 3.6.0 tag.
gep13
force-pushed
the
package-instances-hooks
branch
from
November 5, 2026 22:15
b3a6544 to
a369462
Compare
vexx32
reviewed
Nov 5, 2026
vexx32
left a comment
vexx32
left a comment
Member
There was a problem hiding this comment.
Away to test it out locally, but submitting these comments for now.
| } | ||
|
|
||
| public void ConfigureArgumentParser(OptionSet optionSet, ChocolateyConfiguration configuration) | ||
| public virtual void ConfigureArgumentParser(OptionSet optionSet, ChocolateyConfiguration configuration) |
Member
There was a problem hiding this comment.
Wasn't sure on this one so I did decide to go looking. Apparently this technically can break things, in ways that are possibly rare to hit but quite subtle. C# is a mess 😆
We may not care enough to warrant avoiding this but I thought it was interesting and possibly worth being aware of in future either way:
It looks like the main failure case is "downstream consumer is calling the method directly", which isn't an intended use-case for this, so we're probably okay.
May be worth testing new!CLI with current!CLE to validate that this change doesn't break existing current versions of CLE using the choco export command, at least, if CLE was already overriding the export command?
Comment on lines
+200
to
+204
| /// <summary> | ||
| /// The packages written to the export file. The default is every package | ||
| /// installed under <c>lib</c>; extensions override it to include packages that | ||
| /// live elsewhere. | ||
| /// </summary> |
Member
There was a problem hiding this comment.
Suggested change
| /// <summary> | |
| /// The packages written to the export file. The default is every package | |
| /// installed under <c>lib</c>; extensions override it to include packages that | |
| /// live elsewhere. | |
| /// </summary> | |
| /// <summary> | |
| /// The packages written to the export file. The default is every package | |
| /// installed under <c>lib</c>; this may be overridden to include packages that | |
| /// live elsewhere. | |
| /// </summary> |
Nit / pet peeve: making overly-assertive claims about code that lives elsewhere doing things.
Comment on lines
+189
to
+193
| // Chocolatey-specific nuspec field declaring the package may be installed | ||
| // more than once as named instances (licensed `--instance` feature), read via | ||
| // the NuspecReader.GetSupportsInstances() extension in the NuGet.Client fork. | ||
| // Parsed here (like SoftwareDisplayName / Replaces / ...) so the licensed | ||
| // extension can consume it; OSS CLI itself never acts on it. |
Member
There was a problem hiding this comment.
Similar to prior comment; we have a general pattern of not talking at length about what CLE does in CLI code/comments. Some mention is good but this feels excessive in level-of-detail compared to what we normally do.
It's probably also excessive to mention the detail of what the NuGet.Client code is doing. We may not expect that to change, but if it does there's no chance in hell we're remembering to come back and update a comment all the way over here.
| /// Removes the on-disk store that <paramref name="packageInformation"/> maps to | ||
| /// (see <see cref="GetStorePath(ChocolateyPackageInformation)"/>). | ||
| /// </summary> | ||
| /// <param name="packageInformation">Identifies the store to remove; its Package must be set.</param> |
Member
There was a problem hiding this comment.
This feels like the kind of thing we should have at least a debug assertion around, if not a full ArgumentException if this invariant doesn't hold?
Comment on lines
+42
to
+55
| /// <summary> | ||
| /// Get every installed instance of a package id (for <c>--instance all</c>). | ||
| /// </summary> | ||
| /// <param name="id">The package id.</param> | ||
| /// <param name="config">The configuration.</param> | ||
| /// <returns>All installed instances of the id, ordered by instance name; empty when none.</returns> | ||
| IEnumerable<PackageResult> GetInstalledPackageInstances(string id, ChocolateyConfiguration config); | ||
|
|
||
| /// <summary> | ||
| /// Get every installed package instance across all ids (for <c>--include-instances</c>). | ||
| /// </summary> | ||
| /// <param name="config">The configuration.</param> | ||
| /// <returns>All installed instances, ordered by id then instance name; empty when none.</returns> | ||
| IEnumerable<PackageResult> GetInstalledPackageInstances(ChocolateyConfiguration config); |
Member
There was a problem hiding this comment.
These two comments are referencing params that I don't think are intended to ever be present in CLI; this seems like the wrong place to mention them, to me.
Comment on lines
+1714
to
+1716
| // Packages installed outside lib constrain the resolution as the ones under lib do, but they are not | ||
| // parents to upgrade along with it, and the resolver must never install or remove them. One whose | ||
| // own dependencies are not installed is left out: asking for it would have the resolver install them. |
Member
There was a problem hiding this comment.
This again is more confusing than helpful I fear. Maybe giving an explicit example would illustrate the concept better?
Comment on lines
+67
to
+69
| /// Sets any additional package-level environment variables beyond the standard | ||
| /// ones set by the base implementation. Override in a subclass to inject | ||
| /// extra variables (e.g. ChocolateyPackageInstance for licensed editions). |
Member
There was a problem hiding this comment.
Suggested change
| /// Sets any additional package-level environment variables beyond the standard | |
| /// ones set by the base implementation. Override in a subclass to inject | |
| /// extra variables (e.g. ChocolateyPackageInstance for licensed editions). | |
| /// Sets any additional package-level environment variables beyond the standard | |
| /// ones set by the base implementation. |
Comment on lines
+75
to
+76
| // Base implementation is intentionally empty. | ||
| // Override in a subclass to set additional environment variables. |
Member
There was a problem hiding this comment.
Suggested change
| // Base implementation is intentionally empty. | |
| // Override in a subclass to set additional environment variables. |
We don't typically leave comments like this for this kind of thing, and I don't see a good reason to start here.
Comment on lines
+88
to
+90
| /// Decrypts the remembered arguments stored in <paramref name="argumentsFile"/>, for | ||
| /// callers that keep a package's information somewhere other than the default | ||
| /// <c>.chocolatey\<id>.<version></c> store. |
Member
There was a problem hiding this comment.
Suggested change
| /// Decrypts the remembered arguments stored in <paramref name="argumentsFile"/>, for | |
| /// callers that keep a package's information somewhere other than the default | |
| /// <c>.chocolatey\<id>.<version></c> store. | |
| /// Decrypts the remembered arguments stored in <paramref name="argumentsFile"/>. |
That latter part doesn't seem accurate here.
Member
There was a problem hiding this comment.
I'm not quite clear why these are being put in here. This feels unusual, wouldn't these normally just go in the extension rather than being (I think?) entirely unused things here?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description Of Changes
Adds the extension points that let the Chocolatey Licensed Extension implement
package instances (
choco install <id> --instance <name>), and makes the pagingoptions of
choco listobsolete. Nothing here changes how an ordinary package isinstalled, upgraded, listed or uninstalled. Every new member either defaults to the
current behaviour or is only used by an extension. No existing interface is extended
(new interfaces are added instead), and the changes to existing members widen
accessibility or add overloads and optional parameters.
configuration and fails, naming the property, when a nested property is null. A null
one made every install of a package with an
.exefail in the licensed extension's testharness, with an error that named neither the property nor the builder; the spec catches
the next one at its cause.
ChocolateyPackageMetadata.SupportsInstances,parsed in all three constructors,
falsewhen missing or not a boolean. It is aproperty of the class and is deliberately not added to
IPackageMetadata.ChocolateyPackagePathResolvertakes anoptional instance name: without one paths are unchanged; with one the requested package
resolves to
<root>\<id>\<instance>, and its dependencies resolve to the ordinary layout.instanceattribute onPackagesConfigFilePackageSetting, with specs for the XML mapping and a round trip.InstanceonPackageResultandChocolateyPackageInformation, and the information service reads, saves and removesthrough
Load, a virtualRemoveand a virtualGetStorePath. A newIInstanceAwarePackageInformationServicecarries the instance overloads, soIChocolateyPackageInformationServiceis not extended.SetAdditionalPackageEnvironmentVariableson
PowershellService, and theChocolateyPackageInstancevariable name.NugetService,ChocolateyPackageServiceandChocolateyExportCommandfor where a package is installed,whether it may be, how it is shimmed, where its information is kept, what export writes,
what a package is called in the output (
GetPackageDisplayName), and the dependencies ofpackages installed outside
lib(GetExternalInstalledDependencyInfos), which the install,upgrade and uninstall resolvers use as constraints and never install or remove. Also
IInstanceAwareNugetService, a separate interface for enumerating installed instances.DecryptPackageArgumentsFilethat takes the path of the arguments file,
ArgumentsUtility.GetPackageArgumentsFilefor thedefault path, and a virtual
NugetService.GetRememberedArgumentsFile, so an extension that keepsa package's information elsewhere can have its remembered arguments restored on upgrade.
--pageand--page-sizeare still accepted butignored, with a warning that says they are obsolete (in the log file only under
--limit-output).listonly lists local packages, so there was nothing to page. This is theonly change in behaviour a user of an ordinary install can see.
hooks above the way the extension does, with a service or command that records its calls.
Motivation and Context
Package instances (chocolatey/chocolatey-licensed-issues#468) lets one package be installed more than once, each under a name
chosen with
--instance, for example a Windows service installed several times with differentsettings. The decision is to ship it as a Business feature in the licensed extension, with the
Chocolatey CLI carrying only what that needs. The constraint is that anything added here is
non-breaking, so it can ship in 2.x.
The reason for hooks is that the extension has to change the middle of three large methods
(
NugetService.Install,ChocolateyPackageService.HandlePackageResultand the informationservice's
Get), and none of the decisions inside them had an extension point. Without them theonly way to change those decisions is to copy the methods whole and keep the copies in step. Each
hook is a protected virtual member whose default is today's behaviour, so an extension that does
not override it, and the CLI itself, behave exactly as before.
The CLI never acts on
supportsInstancesor on the instance. They are there for the extension.This depends on the NuGet.Client change that reads
supportsInstances.Testing
Unit specs and integration scenarios are included, and the touched areas are covered with
specs that were each checked by breaking the line they pin:
packages.configmapping,supportsInstancesparsing (all three ways themetadata is read), the package information service, remembered arguments, and the obsolete
paging options, as specs.
NugetServiceInstallSeamScenarios,ExternalInstalledDependencyScenarios(install,upgrade and uninstall resolution with a package outside
lib),ChocolateyPackageServiceSeamSpecs,ChocolateyExportCommandSpecs,PowershellServiceSeamSpecsandNugetServiceSpecs.120 skipped. A further 23 parameterised proxy-configuration cases show as "Not Run" in Test
Explorer; they are not in this change.
Test Kitchen: 50 Pester tests on a Business licence, 0 failed, 2 skipped; the extension's own
unit (2,147) and integration (493) suites pass against it.
Operating Systems Testing
chocolatey-licensed-windows-2022Test Kitchen instance.Change Types Made
Change Checklist
Related Issue
Fixes #3962
Part of chocolatey/chocolatey-licensed-issues#468.