Skip to content

Conversation

@andreiborodin
Copy link
Contributor

TestRunner is a strange asmdef, so added special patching for this project. The AsmDef is referenced by Test projects that always have 'UNITY_INCLUDE_TESTS' defined, and it references nunit which also has it set. However, it itself doesn't and this breaks during generated projects player compilation as no player platform includes that define.

…oject. The AsmDef is referenced by Test projects that always have 'UNITY_INCLUDE_TESTS' defined, and it references nunit which also has it set. However, it itself doesn't and this breaks during generated projects player compilation as no player platform includes that define.
@andreiborodin andreiborodin added the bug Something isn't working label Jan 13, 2020
@andreiborodin andreiborodin added this to the 0.9.0 Release milestone Jan 13, 2020
@andreiborodin andreiborodin self-assigned this Jan 13, 2020
@andreiborodin
Copy link
Contributor Author

This change also has a fix for mapping of build disabled platforms in the solution file.

/// Another patching technique to add defines to some assembly defintion file. TestRunner for example, is only referenced by projects with UNITY_INCLUDE_TESTS and references nunit that has UNITY_INCLUDE_TESTS;
/// However it doesn't have the define itself. This breaks Player build, and as it appears that Unity specially handles this case as well.
/// </summary>
private static readonly Dictionary<string, List<string>> ImpledDefinesForAsmDefs = new Dictionary<string, List<string>>()
Copy link
Contributor

@chrisfromwork chrisfromwork Jan 13, 2020

Choose a reason for hiding this comment

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

ImpledDefinesForAsmDefs [](start = 65, length = 23)

nit: I think this is supposed to be ImpliedDefinesForAsmDefs #Resolved

};

/// <summary>
/// Another patching technique to add defines to some assembly defintion file. TestRunner for example, is only referenced by projects with UNITY_INCLUDE_TESTS and references nunit that has UNITY_INCLUDE_TESTS;
Copy link
Contributor

@chrisfromwork chrisfromwork Jan 13, 2020

Choose a reason for hiding this comment

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

file [](start = 81, length = 4)

nit: some assembly definition file should be some assembly definition files #Resolved

{
asmDefPair.Value.DefineConstraints.Add(define);
}
}
Copy link
Contributor

@chrisfromwork chrisfromwork Jan 13, 2020

Choose a reason for hiding this comment

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

nit: It seems like this should be broken out into a CorrectConstantDefines function #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will rename the entire function, I just think we should loop over asmdefs once.


In reply to: 366027569 [](ancestors = 366027569)

}
}

List<string> availablePlatformsNames = unityProjectInfo.AvailablePlatforms.Select(t => t.Name).ToList();
Copy link
Contributor

@chrisfromwork chrisfromwork Jan 13, 2020

Choose a reason for hiding this comment

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

availablePlatformsNames [](start = 25, length = 23)

nit: availablePlatformsNames rename to availablePlatformNames #Resolved

Copy link
Contributor

@chrisfromwork chrisfromwork left a comment

Choose a reason for hiding this comment

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

:shipit:

@andreiborodin andreiborodin merged commit dbc0e91 into master Jan 13, 2020
@andreiborodin andreiborodin deleted the bugs/patchingTestRunner branch January 13, 2020 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants