Skip to content

fix: Fixed compile failure when compiled against com.unity.nuget.mono-cecil >= 1.11.4 #1920

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

Merged
ShadauxCat merged 6 commits into from
May 9, 2022
Merged

fix: Fixed compile failure when compiled against com.unity.nuget.mono-cecil >= 1.11.4 #1920

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
931cad7
fix: compile fails on Unity 2022.2.0a11 due to breaking API change in…
ShadauxCat Apr 29, 2022
bd4a258
Updated to base version define on Cecil instead of Unity.
ShadauxCat May 7, 2022
d26eeae
changelog
ShadauxCat May 7, 2022
d2522e5
Merge branch 'develop' into fix/compile_failure_with_2022_2_0a12
ShadauxCat May 7, 2022
5622643
Merge branch 'develop' into fix/compile_failure_with_2022_2_0a12
ShadauxCat May 9, 2022
0232970
Merge develop into fix/compile_failure_with_2022_2_0a12
netcode-ci-service May 9, 2022
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
9 changes: 9 additions & 0 deletions com.unity.netcode.gameobjects/Editor/CodeGen/NetworkBehaviourILPP.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,11 @@ private MethodReference GetFastBufferWriterWriteMethod(string name, TypeReferenc
var meetsConstraints = true;
foreach (var constraint in method.GenericParameters[0].Constraints)
{
#if UNITY_CECIL_CONSTRAINTS_ARE_TYPE_REFERENCES
var resolvedConstraint = constraint.Resolve();
#else
var resolvedConstraint = constraint.ConstraintType.Resolve();
#endif

var resolvedConstraintName = resolvedConstraint.FullNameWithGenericParameters(new[] { method.GenericParameters[0] }, new[] { checkType });
if ((resolvedConstraint.IsInterface && !checkType.HasInterface(resolvedConstraintName)) ||
Expand Down Expand Up @@ -737,7 +741,12 @@ private MethodReference GetFastBufferReaderReadMethod(string name, TypeReference
var meetsConstraints = true;
foreach (var constraint in method.GenericParameters[0].Constraints)
{
#if UNITY_CECIL_CONSTRAINTS_ARE_TYPE_REFERENCES
Copy link
Copy Markdown
Contributor

@becksebenius-unity becksebenius-unity Apr 29, 2022

Choose a reason for hiding this comment

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

Curious why use the indirect directive like this instead of just doing UNITY_2022_2_OR_NEWER?

Copy link
Copy Markdown
Collaborator Author

@ShadauxCat ShadauxCat Apr 29, 2022

Choose a reason for hiding this comment

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

Because the breaking change in Unity happened in a patch version. Unity 2022.2.0a11 needs one implementation, Unity 2022.2.0a12 needs another. I'm a little miffed about that. UNITY_2022_2_OR_NEWER was what I wanted to use, but they threw versioning protocol to the wind and broke a patch version so that doesn't work...

Copy link
Copy Markdown
Contributor

@becksebenius-unity becksebenius-unity Apr 29, 2022

Choose a reason for hiding this comment

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

Oh, interesting. Why are we bothering having support for different alpha versions? Shouldn't we just expect people to use the latest alpha? Or at the very least this should be cleaned up after alpha versions are irrelevant.

Maybe you guys are already thinking of this, just wanted to call it out. Thanks for explaining!

Copy link
Copy Markdown
Collaborator Author

@ShadauxCat ShadauxCat Apr 29, 2022

Choose a reason for hiding this comment

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

Honestly I have no idea what's publicly available and what isn't. This just feels like the safest way to do it to me.

Copy link
Copy Markdown
Contributor

@0xFA11 0xFA11 May 3, 2022

Choose a reason for hiding this comment

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

alpha versions are not public, betas are.

Copy link
Copy Markdown
Contributor

@0xFA11 0xFA11 May 3, 2022

Choose a reason for hiding this comment

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

so, we can get away with UNITY_2022_2_OR_NEWER or the next whatever 2022 LTS, we don't need to cover >0.a12 here.

var resolvedConstraint = constraint.Resolve();
#else
var resolvedConstraint = constraint.ConstraintType.Resolve();
#endif


var resolvedConstraintName = resolvedConstraint.FullNameWithGenericParameters(new[] { method.GenericParameters[0] }, new[] { checkType });

Expand Down
11 changes: 9 additions & 2 deletions com.unity.netcode.gameobjects/Editor/CodeGen/com.unity.netcode.editor.codegen.asmdef
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,12 @@
"Mono.Cecil.Pdb.dll",
"Mono.Cecil.Rocks.dll"
],
"autoReferenced": false
}
"autoReferenced": false,
"versionDefines": [
{
"name": "Unity",
"expression": "(0,2022.2.0a11)",
"define": "UNITY_CECIL_CONSTRAINTS_ARE_TYPE_REFERENCES"
}
]
Copy link
Copy Markdown
Contributor

@0xFA11 0xFA11 May 3, 2022

Choose a reason for hiding this comment

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

I checked our internal thread on Slack and looks like the update is coming from com.unity.nuget.mono-cecil package, not the Engine directly. I guess we just updated Editor manifest file such that it points to a newer version of com.unity.nuget.mono-cecil from 2022.2.0a11 and onwards. if so, shouldn't we treat this as a package dep upgrade rather than engine upgrade?

Copy link
Copy Markdown
Contributor

@will-mearns will-mearns May 13, 2022

Choose a reason for hiding this comment

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

Is there a reason why we target a package from an alpha release in the first place? I would think it'd be safest to use whatever was in the last stable version (21 LTS), and then after the next final stable version comes out, we upgrade to that (and in this case implement the if/else)

}