-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Make assembly version logic more robust Fixes #44042 #45518
Make assembly version logic more robust Fixes #44042 #45518
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.
Looks good. Is it possible to add a test case to cover this?
var runtimeFile = !string.IsNullOrWhiteSpace(assemblyPath) && File.Exists(assemblyPath) ? CreateRuntimeFile(referenceProjectInfo.OutputName, assemblyPath) : | ||
!string.IsNullOrWhiteSpace(library.Path) && File.Exists(library.Path) ? CreateRuntimeFile(referenceProjectInfo.OutputName, library.Path) : | ||
new RuntimeFile(referenceProjectInfo.OutputName, string.Empty, string.Empty); |
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.
Do you know if there are cases where each of these clauses are true? (IE will it ever be the case that assemblyPath
doesn't exist but libraryPath
does, or that neither of them exist?
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.
In theory, at least, yes. This gets run as part of a (publicly available/tweakable) Task, so the user can set, for instance, what userRuntimeAssemblies are available --> whether anything gets found for assemblyPath as well as whether a library really exists on disk. This should only be for things that get copied to the output directory, but given that users can customize the inputs, I wouldn't depend on that. Good question.
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 think in a case like this it would be OK to change task logic. The task isn't meant for public consumption, so it's probably OK to make a minor change that probably won't break people even if they are using it.
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 think in a case like this it would be OK to change task logic. The task isn't meant for public consumption, so it's probably OK to make a minor change that probably won't break people even if they are using it.
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.
Totally agree with Daniel here.
Fixes #44042
Replaces the way we look for assembly version.
Without change:

With change:
