Skip to content

Conversation

@cd-work
Copy link
Contributor

@cd-work cd-work commented Nov 27, 2024

This adds support for packages.config files for the C# ecosystem under the new NugetConfig lockfile name.

The file format itself seems to be relatively simple, with just a list of package names and optional versions. The current test fixture is based on examples of this lockfile found on https://github.com.

Both packages.config on its own and *.packages.config are supported as valid names. Since this seems to be a lockfile there is no need for lockfile generation.

@cd-work cd-work requested a review from a team as a code owner November 27, 2024 23:16
@cd-work cd-work requested a review from matt-phylum November 27, 2024 23:16
@cd-work cd-work self-assigned this Nov 27, 2024
@maxrake maxrake self-requested a review November 27, 2024 23:31
@maxrake
Copy link
Contributor

maxrake commented Nov 27, 2024

Can the files be named as *packages.config as well? Or, will there always be a . first (*.packages.config)? I plan to look at this PR in more detail next week, but keeping someone else on the reviewer list who knows C# better is recommended.

@cd-work
Copy link
Contributor Author

cd-work commented Nov 27, 2024

Can the files be named as packages.config as well? Or, will there always be a . first (.packages.config)?

https://github.com/search?q=path%3A**%2F*packages.config+language%3AXML&type=code suggests to me that only packages.config and *.packages.config are valid. This makes sense to me because it matches the new lockfile version and it's just generally sensible.

@matt-phylum
Copy link
Contributor

*.packages.config is not valid.

https://github.com/sichy/XamarinComponents/blob/c1cba5434e8e86859af9b881bea9adaad473e0ec/Util/BuildScripts/ValidateXamarinFormsAndroid/cake.packages.config is renamed to packages.config before use: https://github.com/sichy/XamarinComponents/blob/c1cba5434e8e86859af9b881bea9adaad473e0ec/Util/BuildScripts/ValidateXamarinFormsAndroid/build.sh#L53

https://github.com/DeborahK/AngularF2BWebAPI/blob/4afe42099a1f89dd6196db6ea96f5e91c86f4771/packageFiles/webapi.packages.config#L4 is not copied or renamed, but the repo is not buildable. It's just a collection of instructions for students.

https://github.com/sonatype-nexus-community/DevAudit/blob/ca0a68efacb0e29f817aff5950b0a85bcc5326aa/DevAudit.AuditLibrary/Examples/v6a6.packages.config is an example file from a directory that also contains packages.config.example.

The code for NuGet.exe forms the path to the packages.config file by appending packages.config to the directory name without doing any sort of globbing. https://github.com/NuGet/NuGet.Client/blob/7a2520d1347e3f11f558fc286cc0ba9170973b0e/src/NuGet.Clients/NuGet.CommandLine/Commands/RestoreCommand.cs#L888

However, packages.*.config is valid. Example: https://github.com/GridProtectionAlliance/gsf/blob/10b5b05277d5c5caf70dcec3ac5340ae9cab71f5/Source/Libraries/GSF.Core/packages.GSF.Core.config#L3 NuGet.exe will look for packages.${projectName}.config: https://github.com/NuGet/NuGet.Client/blob/7a2520d1347e3f11f558fc286cc0ba9170973b0e/src/NuGet.Clients/NuGet.CommandLine/Commands/RestoreCommand.cs#L975C31-L992

Counter example: https://github.com/espertechinc/nesper/blob/b3f668904b8bbaf0706ed922fb71c8909990f42e/src/NEsper.Runtime/packages.net45.config#L4 is not a packages.config file because the project name doesn't match. I don't know if we care about checking for a project file in the same directory. It's complicated because the project name put into the packages.*.config pattern is formed by removing the extension from the project file name. To get it exactly right we would either need to replicate NuGet.exe's solution->project->packages.*.config logic or be able to detect if any of the other files in the packages.*.config file's directory are potentially project files, but eligible project files can have many different extensions (NuGet recognizes these, but there could probably be more), and it seems like it'd be better to just have users with packages.*.config files that are not intended to be read as NuGet configure those files to be ignored.

@furi0us333
Copy link
Contributor

I don't know anything about the nuget ecosystem, but I see that the customer sent over a file named farm.packages.config. Unfortunately, I have no context about the rest of the files in their project. Let me know if you would like me to ask for any specific additional information.

@cd-work
Copy link
Contributor Author

cd-work commented Dec 4, 2024

@furi0us333 I've already asked Louis to double-check in the meeting with ICE today. All the evidence points to the file being renamed by ICE, but I'd like to double-check before we move forward.

@furi0us333
Copy link
Contributor

This is the response we received from the customer:

So packages.config is the standard file name but it can be superseded by project.json with NuGet 3.x+. However, there is a PackageReference node in a project file that supersedes them all with NuGet 4.0+. Hope this helps. You can read more about this starting with this document: project.json File Reference for NuGet | Microsoft Learn.

@cd-work cd-work force-pushed the sharp_packages_config branch from 49d9973 to 95c1b21 Compare December 4, 2024 21:05
This adds support for `packages.config` files for the C# ecosystem under
the new `NugetConfig` lockfile name.

The file format itself seems to be relatively simple, with just a list
of package names and optional versions. The current test fixture is
based on examples of this lockfile found on <https://github.com>.

Both `packages.config` on its own and `packages.*.config` are supported
as valid names. Since this seems to be a lockfile there is no need for
lockfile generation.
matt-phylum
matt-phylum previously approved these changes Dec 4, 2024
Copy link
Contributor

@matt-phylum matt-phylum left a comment

Choose a reason for hiding this comment

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

The changelog and some comments still say *.packages.config.

@cd-work cd-work requested a review from matt-phylum December 4, 2024 21:15
matt-phylum
matt-phylum previously approved these changes Dec 4, 2024
@cd-work cd-work requested a review from matt-phylum December 4, 2024 21:22
@cd-work cd-work merged commit 1a45ad3 into main Dec 4, 2024
17 checks passed
@cd-work cd-work deleted the sharp_packages_config branch December 4, 2024 21:38
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.

5 participants