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

fix package path issues when packing content into non-default locations inside nupkg #1135

Merged
merged 1 commit into from
Jan 20, 2017

Conversation

rohit21agrawal
Copy link
Contributor

Fixes: NuGet/Home#4321

Added extensive test coverage for all possible input/output combinations of source path and packagepath.

CC: @rrelyea @emgarten @alpaix @mishra14 @jainaashish @nkolev92 @zhili1208

var setOfTargetPaths = new HashSet<string>(targetPaths, StringComparer.Ordinal);
if (setOfTargetPaths.Remove("contentFiles" + Path.DirectorySeparatorChar)
if (setOfTargetPaths.Remove("contentFiles" + Path.DirectorySeparatorChar)
Copy link
Member

Choose a reason for hiding this comment

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

does this need to check for the AltDirectorySeparatorChar also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, added it .

Copy link
Member

@emgarten emgarten left a comment

Choose a reason for hiding this comment

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

Looks good overall. I suggest testing if using / causes issues on windows since it is looking for \. It is possible someone might author a project using forward slash because they are also working on linux.

foreach (var targetPath in setOfTargetPaths)
{
var newTargetPath = Path.Combine(targetPath, identity);
// We need to do this because evaluuated identity in the above line of code can be an empty string
Copy link
Member

Choose a reason for hiding this comment

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

typo:

because evaluuated identity

-> because the evaluated identity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@rohit21agrawal rohit21agrawal force-pushed the dev-ragrawal-packagepathfix branch from 7e324d5 to 5ca7f78 Compare January 19, 2017 23:48
@rohit21agrawal
Copy link
Contributor Author

@emgarten i added tests with both / and \

@rohit21agrawal rohit21agrawal force-pushed the dev-ragrawal-packagepathfix branch from 5ca7f78 to 11467ac Compare January 20, 2017 00:06
@rohit21agrawal rohit21agrawal merged commit 5335ea1 into dev Jan 20, 2017
@rohit21agrawal rohit21agrawal deleted the dev-ragrawal-packagepathfix branch January 26, 2017 15:18
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.

3 participants