InkPreBuildValidationCheck.AssertNotCompiling() doesn't cause builds to fail when it should

[reallyfancy]

The comments in InkPreBuildValidationCheck.AssertNotCompiling() indicate that this script should cause a build to fail if Ink is currently compiling. Instead, the script just logs an error message to the console. Unity reports the build as successful, with 0 errors.

It looks like as of Unity 2019, regular errors or exceptions don't automatically fail a build. To trigger a build failure, you need to throw a BuildFailedException. There's an example in the docs here: https://docs.unity3d.com/ScriptReference/Build.IPreprocessBuildWithReport.html.

I've reproduced in Unity 6.0, but I imagine this is probably the case in older versions too - haven't had the chance to check!

I can open a PR to fix this if that's helpful. Let me know if so, and which versions you'd need me to test against.

[tomkail]

Oh please do! I've basically not worked on this plugin since the Unity shenanigans so there's likely a few things like this that have set in. Welcome any PRs!

[tomkail]

Oh! And can I suggest trying the Development branch?
https://github.com/inkle/ink-unity-integration/tree/development
It includes this PR #205, which uses a lot more of the modern Unity systems. I've only not merged it in due to lack of testing, but its very likely to be of some use to you!

[reallyfancy]

Ooh, thx - I'll take a look at that!

[reallyfancy]

That PR would fix this issue, because it removes the pre-build validation step. It's obviously a considerably larger change, but I'm happy to put some time towards helping to land that rather than just fixing this issue if that's preferable!

[tomkail]

I see! Are you proposing to add the feature back in, or just leave it out and continue using the dev branch?
Greatly appreciate you testing it out - it's really needed someone with a large ink project to use it and catch any omissions or potential improvements before we go live with it.

[reallyfancy]

Sorry for the slow reply!

It depends on the plans for the dev branch. If there's a plan to get the dev branch merged soon, then my proposed PR would be redundant.

If the importer changes in the dev branch aren't likely to land for a while yet, and you'd want to land a small fix like this directly to main, then I'd be happy to make a PR for it.

What do you reckon?

I'm willing to assist on the dev branch either way, and this fix would be small in scope.

[tomkail]

It sounds like we might be merging it sooner - it's really just needed somebody to test it with a decent size project (I no longer work at Inkle, and they aren't using Unity right now, so our previous approach of being our own testers has collapsed a bit) - so perhaps its one to look at it after the importer. But honestly you're the one doing the work and benefiting from it so I'm happy to follow your lead here!

[reallyfancy]

Great! I'll focus on the ScriptedImporter PR for now. If for any reason that looks like it'll take longer, or there are issues that doesn't fix, then I'll return to this - but all being well, that should resolve this issue too.