Skip to content
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.

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

Do not use relative includes for generated headers. #78342

Closed
wants to merge 1 commit into from

Conversation

jpakkane
Copy link

This commit changed the way includes are done. This causes a problem with generated headers because the include directives now look like this;

#include "../logo_svg.gen.h"

This forces the header to be relative to this source file, i.e. it has to be inside the source directory. This works fine with SCons because it does in-tree builds and writes the .gen.h files inside the source tree. This causes problems for the Meson port, which writes all generated files in a separate build directory so this include directive can not really work.

This patch restores the old behaviour that works with both systems but only for files that are generated during the build.

@jpakkane jpakkane requested review from a team as code owners June 16, 2023 16:52
@jpakkane
Copy link
Author

The issue reported by CI should be fixed now, but this requires a reapproval so the test suite is run again.

Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

@akien-mga Looks good? But I don't understand how we got here.

@akien-mga akien-mga requested review from akien-mga and a team and removed request for a team June 19, 2023 07:56
@akien-mga akien-mga added this to the 4.x milestone Jun 19, 2023
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

Makes sense to me, and the code looks correct.

@akien-mga
Copy link
Member

I'm not fond of reverting that change, as it goes against the goal of making platform ports self-contained and modular, enabling easy plug 'n compile behavior for new / thirdparty platform ports.

We have other places with relative include paths for .gen.h files, why do those work but not the platform ones?

scene/resources/default_theme/default_theme.cpp
34:#include "default_font.gen.h"
35:#include "default_theme_icons.gen.h"

modules/lightmapper_rd/lightmapper_rd.cpp
33:#include "lm_blendseams.glsl.gen.h"
34:#include "lm_compute.glsl.gen.h"
35:#include "lm_raster.glsl.gen.h"

modules/text_server_adv/text_server_adv.cpp
63:#include "icudata.gen.h"

editor/editor_fonts.cpp
33:#include "builtin_fonts.gen.h"

editor/editor_help.cpp
38:#include "doc_data_compressed.gen.h"

If it's only the ../ part that Meson can't handle, then I would suggest changing the code that generates logo_svg.gen.h and run_icon_svg.gen.h so that those get generated in the platform/*/export/ folders instead of platform/*/, since that's the only place where they're used.

@akien-mga
Copy link
Member

If it's only the ../ part that Meson can't handle, then I would suggest changing the code that generates logo_svg.gen.h and run_icon_svg.gen.h so that those get generated in the platform/*/export/ folders instead of platform/*/, since that's the only place where they're used.

Done with #78435, let me know if that solves the issue for the Meson port.

@akien-mga
Copy link
Member

Should be superseded by #78435, but let me know if that's not sufficient and we can rediscuss further changes.

@akien-mga akien-mga closed this Jun 20, 2023
@YuriSizov YuriSizov removed this from the 4.x milestone Jul 24, 2023
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.

6 participants