Skip to content

Misc build system fixes#55792

Merged
akien-mga merged 1 commit into
godotengine:masterfrom
aaronfranke:misc
Dec 10, 2021
Merged

Misc build system fixes#55792
akien-mga merged 1 commit into
godotengine:masterfrom
aaronfranke:misc

Conversation

@aaronfranke

@aaronfranke aaronfranke commented Dec 10, 2021

Copy link
Copy Markdown
Member
  • Fix a bug in GLTF that only showed up on 32-bit x86 Linux but it's applicable to other platforms too. An enum value was given where a bool was expected.
  • Fix sanitizer suffixes being inconsistent. Windows had .s and macOS/Linux had s, now they're all .san.
  • Fix a bug in vhacd that causes it to not build on 64-bit ARM.
  • Fix an if in Android that should be elif.

@akien-mga

akien-mga commented Dec 10, 2021

Copy link
Copy Markdown
Member

Fix sanitizer suffixes being inconsistent. Windows had .s and macOS/Linux had s, now they're all .s.

I'd be more in favor of making Windows consistent with macOS/Linux, which are the platforms where the sanitizers are used most. On Windows only address sanitizer is implemented and only for MSVC.

This minimizes the compat breakage and makes this PR suitable for a 3.x cherry-pick.

Alternatively, if we really want to use a . for separation like for other suffixes, it would probably be worth making it a whole .san to be more explicit. For example a d single letter postfix is common in buildsystems to denote a debug binary, and our s postfix was similar. But a .s postfix looks like ARM assembly.

Either way, this PR doesn't seem to update the CI workflow so I guess it might fail.

@aaronfranke

Copy link
Copy Markdown
Member Author

Sanitization isn't related to architecture, so it would make more sense to me to have .64.san (later can be changed to .x86_64.san) instead of having.64s (later would be changed to .x86_64s).

@akien-mga akien-mga added the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 10, 2021
Comment thread modules/gltf/gltf_document.cpp Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's changing behavior here. ERR_CONTINUE_MSG prints an error, then continue.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should we have ERR_CONTINUE_MSG(true, "Light type is unknown."); or ERR_PRINT("Light type is unknown."); continue;?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's equivalent, I guess we can go with ERR_CONTINUE_MSG(true, ...) to make the intention explicit.

Alternatively it would be:

} else {
    ERR_CONTINUE_MSG(type != "point" && type != "directional", "Light type is unknown.");
}

While at it, could be useful to print type in the error for debugging purposes.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed. ERR_CONTINUE_MSG(true, ...) and adding types makes sense.

@akien-mga akien-mga merged commit 0fba151 into godotengine:master Dec 10, 2021
@akien-mga

Copy link
Copy Markdown
Member

Thanks!

@aaronfranke aaronfranke deleted the misc branch December 10, 2021 21:03
@akien-mga akien-mga removed the cherrypick:3.x Considered for cherry-picking into a future 3.x release label Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants