-
Notifications
You must be signed in to change notification settings - Fork 702
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
Log NU1105 error for badly specified framework #6291
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, not my area of expertise. Could you provide a screenshot of what the command prompt showed before your change?
@@ -153,7 +164,9 @@ private static void ValidateFrameworks(PackageSpec spec, IEnumerable<string> fil | |||
Strings.SpecValidationInvalidFramework, | |||
framework.GetShortFolderName()); | |||
|
|||
throw RestoreSpecException.Create(message, files); | |||
logger.Log(new RestoreLogMessage(LogLevel.Error, NuGetLogCode.NU1105, message)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it in scope of this change to log an error for each invalid target framework and then throw after the foreach
if any were invalid? Not a huge improvement but would be better to tell the user about all invalid frameworks instead of just the first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about that actually. However, framework.GetShortFolderName()
returns "unsupported" instead of the name of the specific framework the user entered. Is there another way to get the name of the fmw name the user entered? I have tried framework.Framework
as well.
If we don't have the specific framework name and we try to aggregate the list of all the problematic fmws the error would end up looking like
Invalid framework : "unsupported", "unsupported", "unsupported"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do something like this:
Hashset<string> invalidFrameworks = new Hashset<string>();
// Verify frameworks are valid
foreach (var framework in frameworks.Where(f => !f.IsSpecificFramework))
{
string frameworkShortFolderName = framework.GetShortFolderName();
if (!invalidFrameworks.Add(frameworkShortFolderName))
{
continue;
}
var message = string.Format(CultureInfo.CurrentCulture, Strings.SpecValidationInvalidFramework,frameworkShortFolderName);
logger.Log(new RestoreLogMessage(LogLevel.Error, NuGetLogCode.NU1105, message));
}
if (invalidFrameworks.Count > 0)
{
throw RestoreSpecException.Create("", files);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that GetShortFolderName() returns "unsupported" instead of the actual framework name that the user provided. This makes it difficult to provide meaningful error messages when logging multiple invalid frameworks.
If we want to log all invalid frameworks before throwing, we need a way to retrieve the exact framework string the user specified. framework.Framework doesn't seem to provide that either.
Do we have another way to access the raw user input for the framework?
If we log each invalid framework individually, the output might look like:
NU1105: Invalid framework: "unsupported"
NU1105: Invalid framework: "unsupported"
NU1105: Invalid framework: "unsupported"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My code is using a hashset to at least de-dupe entries. I don't see anywhere in the code where NuGetFramework.Parse()
stores the original string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see. If we had the original string, that would have been really handy here. Without it, I don't think we should iterate through all the frameworks. We won't be able to provide the user an additional information
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does TargetAlias
contain? Maybe we can report that?
fwiw, NuGet/Home#8174 exists too.
On another note, this doesn't lead to an assets file with an error, so it'd still fail the whole operation.
I'm wondering if we can replicate what VS does?
That might be a bigger piece of work, but worth checking.
Bug
Fixes: NuGet/Home#12943
Description
Ensure misconfigured projects (specifically projects with badly specified framework) fail with a coded error (NU1105), improving error clarity and debugging
PR Checklist