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.

Sign up for GitHub

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

Jump to bottom

Move version check earlier #6674

Merged
merged 5 commits into from
Jul 26, 2021
Merged

Move version check earlier #6674

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7ddd2d2
Move version check earlier
Forgind Jul 14, 2021
c38d9a7
Merge branch 'main' into move-version-check
Forgind Jul 14, 2021
aa75790
Move null check
Forgind Jul 14, 2021
d71e2a5
Merge branch 'move-version-check' of https://github.com/Forgind/msbui…
Forgind Jul 14, 2021
e4d4e0a
Fix comment
Forgind Jul 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 11 additions & 12 deletions src/Tasks/StateFileBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,17 @@ internal static StateFileBase DeserializeCache(string stateFile, TaskLoggingHelp
using (FileStream s = File.OpenRead(stateFile))
{
using var translator = BinaryTranslator.GetReadTranslator(s, buffer: null);

byte version = 0;
translator.Translate(ref version);
// If retVal is still null or the version is wrong, log a message not a warning. This could be a valid cache with the wrong version preventing correct deserialization.
// For the latter case, internals may be unexpectedly null.
Copy link
Member

Choose a reason for hiding this comment

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

Update comment

if (version != CurrentSerializationVersion)
{
log.LogMessageFromResources("General.CouldNotReadStateFileMessage", stateFile, log.FormatResourceString("General.IncompatibleStateFileType"));
Copy link
Member

Choose a reason for hiding this comment

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

I think it continues to make sense for this to be a quiet message. But do we have a test in place that this is not the case for freshly-built-everything using the SDK cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not. If I remember correctly, we decided it would be too hard for some reason.

Copy link
Member

Choose a reason for hiding this comment

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

Can we at least get it added to the manual SDK tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that sounds like a good idea to me. @marcpopMSFT, agree? If so, I can write up some (very detailed) instructions on testing it.

Copy link
Member

Choose a reason for hiding this comment

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

Please reach out to Richa with the scenario you want covered. The difficulty with fresh machine tests as you can only have one.

return null;
}

var constructors = requiredReturnType.GetConstructors();
foreach (var constructor in constructors)
{
Expand All @@ -88,18 +97,8 @@ internal static StateFileBase DeserializeCache(string stateFile, TaskLoggingHelp
retVal = constructor.Invoke(new object[] { translator }) as StateFileBase;
}
}

// If retVal is still null or the version is wrong, log a message not a warning. This could be a valid cache with the wrong version preventing correct deserialization.
// For the latter case, internals may be unexpectedly null.
if (retVal == null || version != CurrentSerializationVersion)
{
// When upgrading to Visual Studio 2008 and running the build for the first time the resource cache files are replaced which causes a cast error due
// to a new version number on the tasks class. "Unable to cast object of type 'Microsoft.Build.Tasks.SystemState' to type 'Microsoft.Build.Tasks.StateFileBase".
// If there is an invalid cast, a message rather than a warning should be emitted.
log.LogMessageFromResources("General.CouldNotReadStateFileMessage", stateFile, log.FormatResourceString("General.IncompatibleStateFileType"));
return null;
}
else if (!requiredReturnType.IsInstanceOfType(retVal))

if (retVal == null || !requiredReturnType.IsInstanceOfType(retVal))
{
log.LogMessageFromResources("General.CouldNotReadStateFileMessage", stateFile,
log.FormatResourceString("General.IncompatibleStateFileType"));
Expand Down