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

(#508) Attempt to resolve relative paths for sources #2943

Merged
merged 4 commits into from
Jan 9, 2023

Conversation

TheCakeIsNaOH
Copy link
Member

@TheCakeIsNaOH TheCakeIsNaOH commented Dec 20, 2022

Description Of Changes

When setting up NuGet sources, if NuGet is unable to parse the path, this tries to resolve relative paths to absolute, and then retries setting up the NuGet source. If the path is unable to be resolved, then this ignores the error and lets NuGet handle the resulting invalid source.

This path resolution requires the filesystem to be passed in. This means that various method signatures change to allow access to the filesystem.

Motivation and Context

When using NuGet.Core, then relative paths were able to be resolved. But now with NuGet.Client, relative paths are resolved by clients, not by libraries, which is why this change has to be made. NuGet/NuGet.Client#3783

Testing

  • Add wget .nupkg to C:\packages
  • Open PowerShell at C:\
  • Run choco.exe from this PR with .\choco.exe info wget --source="C:\packages"
  • Run choco.exe from this PR with .\choco.exe info wget --source="packages"
  • Change directory to C:\packages
  • Run choco.exe from this PR with .\choco.exe info wget --source="."
  • Validate that all of these output the wget info
  • Rerun these commands, but with install (with -n -f) instead of info, and validate that the package installs each time.

Operating Systems Testing

  • Windows 10 22H2

Change Types Made

  • Bug fix (non-breaking change).
  • Feature / Enhancement (non-breaking change).
  • Breaking change (fix or feature that could cause existing functionality to change).
  • Documentation changes.
  • PowerShell code changes.

Change Checklist

  • Requires a change to the documentation.
  • Documentation has been updated.
  • Tests to cover my changes, have been added.
  • All new and existing tests passed?
  • PowerShell code changes: PowerShell v2 compatibility checked?

Related Issue

Part of #508

@TheCakeIsNaOH TheCakeIsNaOH force-pushed the relative-path-source branch 2 times, most recently from 418f3a3 to 0379cdf Compare December 20, 2022 20:50
@TheCakeIsNaOH
Copy link
Member Author

[Fact]
public void should_log_package_information()
{
var lastWriteDate = File.GetLastWriteTimeUtc(Path.Combine("PackageOutput", "installpackage.1.0.0" + NuGetConstants.PackageExtension))
.ToShortDateString();
MockLogger.Messages.Keys.ShouldContain(LogLevel.Info.to_string());
MockLogger.Messages[LogLevel.Info.to_string()].ShouldContain(" Title: installpackage | Published: {0}\r\n Number of Downloads: n/a | Downloads for this version: n/a\r\n Package url\r\n Chocolatey Package Source: n/a\r\n Tags: installpackage admin\r\n Software Site: n/a\r\n Software License: n/a\r\n Summary: __REPLACE__\r\n Description: __REPLACE__\r\n".format_with(lastWriteDate));
}

This test is failing, and I'm not quite sure why. Comparing the actual output vs the expected output, they appear to be the same.

@TheCakeIsNaOH TheCakeIsNaOH marked this pull request as ready for review December 20, 2022 21:08
@choco-bot
Copy link

@TheCakeIsNaOH TheCakeIsNaOH force-pushed the relative-path-source branch 2 times, most recently from 48380e8 to a8242f3 Compare December 22, 2022 15:59
@TheCakeIsNaOH
Copy link
Member Author

I've tweaked the info output to make it the same as it was with NuGet.Core, so as to make the integration test pass.

gep13
gep13 previously approved these changes Jan 6, 2023
Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gep13
Copy link
Member

gep13 commented Jan 6, 2023

@TheCakeIsNaOH the unit tests seem to be failing on Posix. Is this something that you can take a look at?

When setting up NuGet sources, if NuGet is unable to parse the path,
this tries to resolve relative paths to absolute, and then retries
setting up the NuGet source. If the path is unable to be resolved, then
this ignores the error and lets NuGet handle the resulting invalid
source.

This path resolution requires the filesystem to be passed in. This
means that various method signatures change to allow access to the
filesystem.

When using NuGet.Core, then relative paths were able to be resolved.
But now with NuGet.Client, relative paths are resolved by clients,
not by libraries, which is why this change has to be made.
NuGet/NuGet.Client#3783
This adds tests to ensure that NuGet is able to parse various types of NuGet
sources correctly. This includes http, https, and local paths, where local
paths include absolute, different types of relative paths, and UNC.
This makes the info output the same as it was when using NuGet.Core so
as to keep the info output the same as it is with v1.x
@TheCakeIsNaOH
Copy link
Member Author

@gep13 I've updated this, and it is ready for review again.

Copy link
Member

@gep13 gep13 left a comment

Choose a reason for hiding this comment

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

LGTM!

@gep13 gep13 merged commit 1d33db8 into chocolatey:develop Jan 9, 2023
@gep13
Copy link
Member

gep13 commented Jan 9, 2023

@TheCakeIsNaOH thanks for getting this updated!

@TheCakeIsNaOH TheCakeIsNaOH deleted the relative-path-source branch January 9, 2023 14:40
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.

None yet

3 participants