Skip to content
This repository was archived by the owner on Dec 18, 2017. It is now read-only.

"kpm restore" ignores nonexistent local package feeds and gives warnings #1032

Merged
merged 1 commit into from
Jan 12, 2015

Conversation

ChengTian
Copy link
Contributor

parent #845

kpm restore gives warning instead of blowing up now.
untitled

Also extracted some shared code to have DRYer code.

{
_source = source;
IsHttp = isHttp;
IsHttp = IsHttpSource(source);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving initialization of IsHttp here can simplify extraction of shared code (PackageSourceUtils.CreatePackageFeed()). Moreover, the source argument already contains IsHttp information, so passing isHttp introduces duplicated info.

@ChengTian ChengTian force-pushed the kpm-restore-from-nonexistent-local-package-source branch from f17d1ab to 6488d09 Compare January 5, 2015 23:28
@ChengTian
Copy link
Contributor Author

@davidfowl , thanks for the feedbacks. Please check the latest commits.

@Praburaj
Copy link
Contributor

Praburaj commented Jan 6, 2015

Is this PR related to this bug? #1018 ?

@Praburaj
Copy link
Contributor

Praburaj commented Jan 6, 2015

The issue addressed with this PR and the bug #1018 seems to be similar/same except the difference of local or remote source. If --ignore-failed-sources needs to be used for http sources to ignore on failure it will be good to have the same behavior for local sources.

@ChengTian
Copy link
Contributor Author

Talked to @Praburaj and here is a summary:
If we take the current PR, local feeds and remote feeds will have inconsistent behaviors.
Local feeds: if not accessible, always show warning and skip it
Remote feeds: if not accessible, kpm restore exits with errors; if not accessible but --ignore-failed-sources is given, show warning and skip it

I think @Praburaj is right. It's ideal if local feeds and remote feeds have consistent behaviors. However, with a nonexistent folder, we cannot determine type of the local feed (KpmPackageFolder or NuGetPackageFolder) so we cannot proceed to the next step of kpm restore. To have the same behavior as remote feeds, we can create a class NonexistentPackageFolder : IPackageFeed, in which FindPackagesByIdAsync throws exception when ``--ignore-failed-sources` is not given.

@ChengTian ChengTian force-pushed the kpm-restore-from-nonexistent-local-package-source branch 2 times, most recently from aac16f6 to 7464ecb Compare January 9, 2015 01:17
@ChengTian
Copy link
Contributor Author

Made behaviors of local feeds more consistent with remote feeds:

  • When kpm restore from nonexistent local feed:
    capture2
    capture3
  • When kpm restore --ignore-failed-sources from nonexistent local feed, we give a warning and ignore the failed feed:
    capture1

@Praburaj
Copy link
Contributor

Praburaj commented Jan 9, 2015

Can we highlight the warning message with a different color code. The message looks buried a bit.

@ChengTian
Copy link
Contributor Author

@Praburaj , the output above is using the same color code as remote feed error messages. If we want to change the color code, we should change both local and remote feeds in a separate PR. Please create an issue for it if you feel strongly. Thanks.

@davidfowl
Copy link
Member

:shipit:


namespace Microsoft.Framework.PackageManager.Restore.NuGet
{
public class NonExistentPackageFolder : IPackageFeed
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this ehhhh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the structure of a folder doesn't conform with NuGetPackageFolder, we always assume it is a KpmPackageFolder. So I remove NonExistentPackageFolder and move the logic to KpmPackageFolder. Do you like this approach? b34cda0

@Praburaj
Copy link
Contributor

Praburaj commented Jan 9, 2015

@ChengTian sure I filed #1051

@davidfowl
Copy link
Member

I guess it's fine, :shipit:

- '--ignore-failed-sources' also applies to local feeds now
@ChengTian ChengTian force-pushed the kpm-restore-from-nonexistent-local-package-source branch from b34cda0 to 8d18771 Compare January 12, 2015 17:15
@ChengTian ChengTian merged commit 8d18771 into dev Jan 12, 2015
@ChengTian ChengTian deleted the kpm-restore-from-nonexistent-local-package-source branch January 12, 2015 17:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants