FIX: separated situation of building/installing on target_include_directories on cmake

[Dalmurii]

Basically it is not about "making export enabled". I am talking about a simple policy that included directories(for headers) must not be children of where root CMakeLists.txt is when being exported.
This check is hard-coded behaviour not to confuse the system, since installation in this context stands for a situation that several wanted files(headers, library files, etc) should move to another path then original.

Since we are not making any exportation of godot-cpp itself, you do not need to understand those to understand this request I am writing.

For detail, my stupid macro generator interface library is marked (could be installed) refers to godot-cpp, which expects godot-cpp to separate the directory to include.

Reading first sentence of this paragraph would help in this case.

[enetheru]

@Dalmurii
I'm happy to approve code that I understand the reason for, so I want you to imagine what it's like to receive a change with no description, no link to an issue, no conversation, etc. What appears obvious to you is not obvious to others, so without any context, what is one to understand from this change? What problem does it solve? What does it enable?

So for now, I await an update.

[Dalmurii]

@enetheru

Sorry for lack of explanation.

CMake strictly requires the separation when adding inclusion for directories for C/C++.

Directory to include should not be prefixed by source (where the CMakeLists.txt) for installation, since guaranteeing installing will happen on source is barely possible.

Existing source was including full path of directory without condition, and that breaks the exportation of target being installed referring to it according to above.

I have branched it by two with generator expressions to fix this behaviour.

[enetheru]

We don't use any installation things at all right now, for reasons. This would be the first piece of code that explicitly caters to installation if merged.

Can you tell me about your extension workflow and how you currently use installation in that workflow?

[Dalmurii]

We don't use any installation things at all right now, for reasons. This would be the first piece of code that explicitly caters to installation if merged.

Can you tell me about your extension workflow and how you currently use installation in that workflow?

While installation is not possible in godot-cpp, one might want to export this project with his own.

I am the one trying this and faced this.

[Dalmurii]

Following is problematic on my side:

target_link_libraries(libdne-common # generated one with install
INTERFACE
	godot-cpp            # exported
	ae2f::Core 
	ae3f::met2d
	)

# godot-cpp now can be installed.
# tries also finding directories to include
# crashes when source-prefixed.
install(TARGETS godot-cpp EXPORT commonTargets)

Like this, godot-cpp could be installed despite the lack of installation script.

@enetheru

[enetheru]

OK excellent, thanks for providing more info.

If I am understanding you correctly, you're producing middle-ware using godot-cpp, that would be consumed by a third party(could also be your own project). CMake export workflow requires separation of the build/install to work properly.

Is that about right? I have to spend some time re-learning the install/export/import workflow that a lot of CMake user use.
Things like superbuilds, and in house pre-build distributions and the like could be supported with some changes like yours.

These things have been requested in the past, only to be shut down because they act too much like installs and we don't want package maintainers to start packaging godot-cpp. While we still don't want package maintainers to create packages, supporting the widely used export/import workflow is now OK.

I just haven't spent the time to really get stuck into it. I'll try to block out some time, so if you have any resources that will shorten my learning curve, feel free to link them.

[enetheru]

Since we don't have any other install/export based code in the repo, would making the files BUILD_INTERFACE only still work for you? Or do you need the INSTALL_INTERFACE for your project?

[Dalmurii]

I would check as soon as possible and if remove install interface part when not strictly necessary

[Dalmurii]

I have not seen above truely sorry, I am now responding it.
Basically it is not about "making export enabled". I am talking about a simple policy
that included directories(for headers) must not be children of where root CMakeLists.txt is when being exported.
This check is hard-coded behaviour not to confuse the system, since installation in this context stands for a situation that several wanted files(headers, library files, etc) should move to another path then original.

Since we don't have any other install/export based code in the repo, would making the files BUILD_INTERFACE only still work for you? Or do you need the INSTALL_INTERFACE for your project?

Turns out INSTALL_INTERFACE is not required since I am making a shared object and that needs no header to run for my use case. I am bloody idiot. I will remove any changes related to adding INSTALL_INTERFACE.

If I am understanding you correctly, you're producing middle-ware using godot-cpp, that would be consumed by a third party(could also be your own project). CMake export workflow requires separation of the build/install to work properly.

You are understanding correctly. For detail, my stupid macro generator interface library is marked (could be installed) refers to godot-cpp, which expects godot-cpp to separate the directory to include.

have to spend some time re-learning the install/export/import workflow that a lot of CMake user use. Things like superbuilds, and in house pre-build distributions and the like could be supported with some changes like yours.

So since we are not making any exportation of godot-cpp itself, you do not need to understand those to understand this request I am writing.

I just haven't spent the time to really get stuck into it. I'll try to block out some time, so if you have any resources that will shorten my learning curve, feel free to link them.

Reading first sentence of this paragraph would help in this case.

[enetheru]

Great, I'm glad I we seem to have an understanding, I'll approve the merge now as the changes are appropriate, just squash the commits, and we'll get someone to approve the workflow.

Cheers.
@dsnopek, @Ivorforce can we get an approval for the workflow? I don't have access.

[Dalmurii]

Commits has been squashed.

[enetheru]

Preventing the fully qualified paths from ending up in middle ware extensions that do want to perform install/export is a good thing.

There are probably more things that we can to towards this end, but for now this solves a real problem for someone so I approve.

[enetheru]

some additional changes have crept in that sholdnt be there.

[Dalmurii]

I think I've falsely squashed? I will try staging only one file manually

[enetheru]

And if you can edit the description for anyone who views in the future that would also be good, cheers.

[Dalmurii]

I have not seen above truely sorry, I am now responding it. Basically it is not about "making export enabled". I am talking about a simple policy that included directories(for headers) must not be children of where root CMakeLists.txt is when being exported. This check is hard-coded behaviour not to confuse the system, since installation in this context stands for a situation that several wanted files(headers, library files, etc) should move to another path then original.

Since we don't have any other install/export based code in the repo, would making the files BUILD_INTERFACE only still work for you? Or do you need the INSTALL_INTERFACE for your project?

Turns out INSTALL_INTERFACE is not required since I am making a shared object and that needs no header to run for my use case. I am bloody idiot. I will remove any changes related to adding INSTALL_INTERFACE.

If I am understanding you correctly, you're producing middle-ware using godot-cpp, that would be consumed by a third party(could also be your own project). CMake export workflow requires separation of the build/install to work properly.

You are understanding correctly. For detail, my stupid macro generator interface library is marked (could be installed) refers to godot-cpp, which expects godot-cpp to separate the directory to include.

have to spend some time re-learning the install/export/import workflow that a lot of CMake user use. Things like superbuilds, and in house pre-build distributions and the like could be supported with some changes like yours.

So since we are not making any exportation of godot-cpp itself, you do not need to understand those to understand this request I am writing.

I just haven't spent the time to really get stuck into it. I'll try to block out some time, so if you have any resources that will shorten my learning curve, feel free to link them.

Reading first sentence of this paragraph would help in this case.

Would summarising this enough?

[enetheru]

LGTM

[enetheru]

Would summarising this enough?

good plan.

[Dalmurii]

Checks seems to be failing? I will try resolving gersemi thing.

[Ivorforce]

I recommend installing pre-commit and just running it locally. Saves you from guessing the correct indentation / style to use.

[Dalmurii]

I recommend installing pre-commit and just running it locally. Saves you from guessing the correct indentation / style to use.

Oh this is very convenient