mesonbuild / meson

The Meson Build System
http://mesonbuild.com
Apache License 2.0
5.6k stars 1.63k forks source link

Qt compile_moc() doesn't support files with the same name #10955

Open punytroll opened 2 years ago

punytroll commented 2 years ago

Describe the bug

When using Qt's compile_moc(), files that have the same file name, while being placed in different sub directories in the source tree, produce a configure-time error:

ERROR: Multiple producers for Ninja target "app.p/moc_value.cpp". Please rename your targets.

or

ERROR: Multiple producers for Ninja target "app.p/value.moc". Please rename your targets.

depending on whether one is using the headers or sources keyword argument.

To Reproduce

This minimal meson.build file:

project('bug', 'cpp')
qt = import('qt6')
app_sources = files('app.cpp')
app_sources += qt.compile_moc(sources: [
    'one/value.h',
    'two/value.h'])
executable('app', sources: app_sources)

Files one/value.h and two/value.h must exist, but can, for this error at configure time, be empty.

Expected behavior

There is no technical reason, why this shouldn't work. CMake can do it and a handcrafted Makefile certainly can as well. The moc files need either be placed in separate directories (CMake creates unique build directories for this) and the include paths to these directories have to be provided at compile time. Or the filenames have to be prefixed with the directory ('one_value.moc' / 'two_value.moc'), which seems more mesonic, but creates a problem when you want to include these files again ('#include "one_value.moc"' instead of the canonic '#include "value.moc"' in both files, trusting the include paths to be correct.)

system parameters

punytroll commented 2 years ago

Updated to meson 0.63.3. Problem remains.

tristan957 commented 2 years ago

Given a non-qt project:

srcdir
├── include
│   ├── hse
│   │   └── pidfile
│   │       └── pidfile.h
│   └── meson.build
├── lib
│   ├── meson.build
│   └── pidfile.c
└── meson.build
builddir
├── include
├── lib
├── libhse-pidfile.a
└── libhse-pidfile.a.p
    └── lib_pidfile.c.o

You can see that the folder name is prefixing the pidfile.c name in the private dir. I think all the moc stuff needs to do is something similar

tristan957 commented 2 years ago

The bug is located right around here https://github.com/mesonbuild/meson/blob/master/mesonbuild/modules/qt.py#L463

punytroll commented 2 years ago

Aye, that's what I thought. Is it open for debate whether the prefix is embedded in the filename or in a directory structure?

Because, as mentioned in the issue, I recognize that something like "one_value.moc" would be more mesonic, but that is not what Qt developers would expect! Traditionally, these generated moc files would be #included as "value.moc" inside of the cpp file. However, if the filename is extended with directory prefixes that doesn't work anymore ...

So, would an implementation for this issue be free to create subdirectories inside the private directory and append those to the include directories?

I hope I'm making myself clear enough ... ;)

tristan957 commented 2 years ago

So you have something like this?

src/a/meson.build compiles moc.

src/b/meson.build compiles moc.

And both generate the same file name in builddir/?

punytroll commented 2 years ago

Yes, even if I place individual meson.build files inside the subdirectories one and two it wants to create two files app.p/value.moc. I've tried that before but forgot about that.

However, I am convinced that both approaches should be handled correctly by meson:

  1. placing individual meson.build files with compile_moc() in the subdirectories
  2. placing a single meson.build file with a combined compile_moc() in any parent directory
jpakkane commented 2 years ago

Regardless of everything else, I highly recommend adopting a naming scheme that prohibits having multiple headers of the same name. Murphy's law says that eventually you will get that wrong and include the wrong header from the wrong place. The only real protection against that is to have unique names.

tristan957 commented 2 years ago

@punytroll the obvious solution to me seems to be generating the files into builddir/{a,b}/ instead of builddir.

punytroll commented 2 years ago

In general, that might be a sensible recommendation.

On the other hand, structuring your code in subdirectories accoring to your namespace names and files according to your class names is not that absurd an idea. And it's only a matter of time until some (dreaded) Manager, Parser or Value class comes along ... If the developer team gets big enough or the project gets complex enough, I don't think you can sensibly enforce this rule.

And in any case, namespaces provide good protection against accidentally including and using the wrong header without the compiler complaining. I don't think protecting against that falls into the realm of responsibilities for the build system.

punytroll commented 2 years ago

@tristan957 I think so as well, but I wanted to make sure, that subdirectories in the build directory are an OK thing to do.

I will have a stab at it.

tristan957 commented 2 years ago

@punytroll generally writing output files to the mirrored build directory is the right thing to do

eli-schwartz commented 2 years ago

Yes, even if I place individual meson.build files inside the subdirectories one and two it wants to create two files app.p/value.moc. I've tried that before but forgot about that.

app.p is a per-target private directory.

If this were handled via manual running of generator().process() with find_program('moc') as the command to run, then all files will be generated into process() has a preserve_path_from: kwarg, which allows the input one/value.h to be transformed into app.p/one/moc_value.cpp; this is fine, because although Meson doesn't generally allow outputting files into subdirectories, it allows it when operating inside the private directory for a target.

It's perfectly reasonable to argue that the builtin moc should always do that too.

tristan957 commented 2 years ago

Would make sense to me

jpakkane commented 2 years ago

Or it could have the preserve kwarg so people could opt into it. Changing the default would break things for existing projects.

eli-schwartz commented 2 years ago

Since we are talking about generated .cpp files, the risk of that is probably pretty low... although there's no harm in making it optional I guess.

jpakkane commented 2 years ago

Hmm. Wait, does moc produce only source files or headers too? If the latter then we can't change the output names, but if it is only the former then probably yes.

punytroll commented 2 years ago

Well, that depends on how you look at that:

  1. when using compile_moc() with the headers keyword, then the output is a cpp file, internal to the meson build process, that meson automatically compiles and links into the target. The problem of this issue applies here but the actual filename is never exposed or referred to.
  2. when using compile_moc() with the sources keyword, then the output is a .moc file that needs to be included manually in the cpp source file. The moc file, though being an implementation file, is #included, so behaviorally it is a header file as well. The problem of this issue applies as well.

In the latter case, the name of the generated file should definitely not be changed from the expected file name (<basefilename>.moc), because that file name is probably referred to in another source file. The directory of the generated file can be changed of course, as long as the resulting directory is appended to the include_directories of the target.

punytroll commented 2 years ago

I have to agree with @eli-schwartz: As this pertains to generated files only, is there any chance it could break something for existing projects? As long as the created subdirectories are appended to the target's include_directories, the same file names should be visible to the compiler to include ...

punytroll commented 2 years ago

I had a quick glance at this:

It seems passing preserve_path_from = state.environment.source_dir to Generator.process_files() works quite nicely. But only when the headers kwarg is used: the generated .cpp files are compiled and linked by meson automatically, which evades any necessity to care about the paths they are generated into. This resolves the issue nicely ...

On the other hand, when using the sources kwarg, the moc tool generates .moc files. Those are meant to be #included in a .cpp file, which makes it necessary to add the correct -Ibuild/dir flag to the correct .cpp file compiling command. I'm not sure whether this is possible or even wanted ...

Any thoughts on this? Should I provide a tiny PR with passing the preserve_path_from argument?

tristan957 commented 2 years ago

I think that is very reasonable.

bruchar1 commented 1 year ago

I have an open PR that adds the preserve_paths keyword argument: #11795.