Include order define leaked from 5.7 back to 5.6

In 5.6.0, there are 8 files in Core\Public\HAL that check UE_ENABLE_INCLUDE_ORDER_DEPRECATED_IN_5_7, which is not yet defined. It seems like if it _was_ defined in 5.6, the value would be 1, because the include order in question is not yet deprecated. But since it’s not defined, it evaluates to 0. This doesn’t seem to be preventing us from compiling the engine in 5.6.0, but I’m still unsure if the intended behavior is happening.

The reason this wasn’t caught is also interesting and should probably be changed. Prior to 5.6, UndefinedIdentifierWarningLevel in TargetRules.cs defaulted to WarningLevel.Error, but after the warning/error refactor to CppCompileWarnings.cs, the new default is WarningLevel.Off. Setting it back to Error is how I found this issue. This happened about 6 weeks before the issue with this define.

Assuming I’d like to set it back to Error (whether the engine at large does so or not), is there a recommendation on how to deal with this issue? Ideally I’d like to avoid adding the new engine include order enum entry myself ahead of 5.7, but it also feels bad to define it to 1 in some arbitrary place with an assert to remind myself to revert the change when 5.7 comes out. Maybe this could be fixed in 5.6.1, but I don’t know what the timeline is like there. I could also grab a change from Perforce or Github early if it was fixed on the Epic side before the next version was scheduled to release. Please let me know. :slight_smile:

Steps to Reproduce
In 5.6, change UndefinedIdentifierWarningLevel’s WarningLevelDefault to WarningLevel.Error.

Unfortunately, we will not be patching those 5.7 macros because of our policy of not modifying public headers in a hotfix. If you want to turn on UndefinedIdentifierWarningLevel, please use a workaround for this issue. I understand your concern about forgetting to remove your own definition of the macro. An alternative is to change the 8 affected files to use the 5_6 macro instead of the 5_7 macro. When I made that change, I didn’t realize we would be merging it into 5.6, but since we did it should have used the 5_6 macro. I’ll follow up on how the default for UndefinedIdentifierWarningLevel changed, because it looks like it was meant to stay as Error.

Hey there,

Just to chime in - I will be looking at this UBT UndefinedIdentifierWarningLevel being set back to default to Error, as well as the constituent engine fixes. To Devin’s point, this will not be feasible for a 5.6.X due to public headers. I will aim to get this in for 5.7. Upon submitting, I will place the github change here to cherrypick should you choose to do so.

Kind regards,

Julian

Hey there,

Just wanted to provide an update that I have been chipping away at this over the past couple weeks and expect to share a set of changelists that you can examine for your own local changes.

Kind regards,

Julian

Just adding another comment here to keep the ticket from being closed (I think). [mention removed]​ , not sure if you have anything to share yet. If you have some changes but not all, I’d still consider taking some piecemeal for our codebase, but I understand if you want to post them all at once.

Much appreciated!

The change to officially switch the flag just went in over the weekend - so I am now just lurking to make sure we caught everything. After this I’ll be gathering applicable changes for 5.6 for you - sometime this week :slight_smile: Appreciate the patience here, as I’m sure you can imagine I had some house-keeping to tend to with this one hah!

Kind regards,

Julian

Feel free to tell me to make another ticket, but I found what seems like another bug in this same vein. Thankfully, it didn’t seem to introduce any new errors for us when I fixed it locally.

Prior to 5.6, ModuleRules.DeterministicWarningLevel defaulted to Warning if Target.bDeterministic was true. Now, CppCompileWarnings.DeterministicWarningLevel still checks if this is a deterministic build via StandardFilters.DeterministcFlagSetFilter but defaults to WarningLevel.Off. So if it is deterministic, and not overridden, then the default is applied, which is Off. I was able to confirm this behavior by adding some code that would cause the warning and compiling Shipping without issue; then I set the default to warning, and it was hit on the next build.

Appreciate bringing this up. I’ll have a look at this as well to make sure.

Would be good to see this fixed, but as a workaround, you could add this to your .Target.cs:

if (BuildEnvironment == TargetBuildEnvironment.Unique)
{
	// Workaround issue with some 5.6 code testing this macro from 5.7
	GlobalDefinitions.Add("UE_ENABLE_INCLUDE_ORDER_DEPRECATED_IN_5_7=0");
}

This is kind of what I’m hoping to avoid, though we’ll see, I suppose. I believe the value should be 1, though, since theoretically we do want to enable the include order that has not been deprecated yet. That’s kind of the interesting thing, the existing code seems wrong because not being defined is the same as 0, and this include order doesn’t exist yet.

Also, we would need this in shared build environments as well, not just unique. That’s maybe not an interesting point, though.

You’re correct, but it seems that it compiles fine for me when set to 0 :slight_smile:

The main thing I wanted was to be able to enable back the warning as error, and changing GlobalDefinitions in shared build environments cause an error because of mismatching the ones from Unreal’s target.cs, which I can’t do since I wanted to try and use the installed vanilla engine.

You’re right, I forgot GlobalDefinitions can’t be modified in a shared build environment. We build the engine from source and would not use a unique environment for our game’s Editor, so we’d have to change it in all the shared targets. I’d certainly like to avoid that. I think we’re more likely to leave the setting disabled and see what a fix from Epic might look like.

Thank you for the update. I made a local change similar to Devin’s suggestion, where I changed the 5.7 macros to 5.6 so I could re-enable the error. After doing so, there were a handful more issues that I had to resolve, and I expect those are the ones you’re getting fixed the right way. We did get to a point where everything compiled, but I still look forward to reverting my changes and taking yours. :slight_smile: