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

Auto header dependencies don't work with generated enums #1084

Closed
ssssam opened this issue Nov 22, 2016 · 21 comments
Closed

Auto header dependencies don't work with generated enums #1084

ssssam opened this issue Nov 22, 2016 · 21 comments

Comments

@ssssam
Copy link
Contributor

ssssam commented Nov 22, 2016

Given this meson.build file:

project('meson-enums-dependencies', 'c')

gnome = import('gnome')

enums = gnome.mkenums('enum-types',
    sources: 'enums.h',
    c_template: 'enum-types.c.template',
    h_template: 'enum-types.h.template'
)

lib1 = library('lib1', enums[0], enums[1])

executable('final', 'main.c', link_with: lib1)

When I build this with a clean build directory, I get the following error:

[0/1] Regenerating build files
The Meson build system
Version: 0.37.0.dev1
Source dir: /home/sam/meson-enums-fuckup
Build dir: /home/sam/meson-enums-fuckup/build
Build type: native build
Project name: meson-enums-dependencies
Native c compiler: ccache cc (gcc 6.2.1)
Build machine cpu family: x86_64
Build machine cpu: x86_64
Build targets in project: 4
[1/7] Compiling c object 'final@exe/main.c.o'
FAILED: final@exe/main.c.o 
ccache cc  '-Ifinal@exe' '-fdiagnostics-color=always' '-I.' '-I..' '-pipe' '-Wall' '-Winvalid-pch' '-O0' '-g' '-MMD' '-MQ' 'final@exe/main.c.o' '-MF' 'final@exe/main.c.o.d' -o 'final@exe/main.c.o' -c ../main.c
../main.c:1:24: fatal error: enum-types.h: No such file or directory
 #include "enum-types.h"
                        ^
compilation terminated.
[2/7] 'Generating enum-types.h with a meson_exe.py custom command.'

I can work around this issue by listing enums[1] as a source to the executable. But that's no good; imagine if I have 50 testcases for lib1 -- I shouldn't have to list the enum header file as a source for each of those testcases.

@rhd
Copy link
Contributor

rhd commented Nov 22, 2016

make a lib1 dependency.

lib1_inc = include_directories('.')
lib1 = library('lib1', enums[0], enums[1])
lib1_dep = declare_dependency(
    sources: enums,
    include_directories: lib1_inc,
    link_with: lib1,
)

executable('final', 'main.c', dependencies: lib1_dep)

... something like that should work.

@ssssam
Copy link
Contributor Author

ssssam commented Nov 22, 2016

Aha, that does work, thanks.

This still isn't ideal; at minimum the docs at https://github.com/mesonbuild/meson/wiki/Gnome-module for mkenums need to mention that this needs to be done for every library that uses generated enums (I'd send a patch but I'm not sure how to do that for the wiki). Ideally Meson would be able to work out the dependency ordering for itself.

@TingPing
Copy link
Member

TingPing commented Nov 22, 2016

This isn't simply a mkenums problem. Meson tracks dependencies so you have to be explicit about what depends upon what. Your executable depends upon a CustomTarget that generates a header and a library. The library and header are two different unrelated things until you tie them together with declare_dependency. Alternatively you can just shove the header into the sources list of the executable.

EDIT: Sorry I see that the lib should be generating the enums in the first place, that might be a bug.

@ssssam
Copy link
Contributor Author

ssssam commented Nov 22, 2016

Yeah; to me it seems reasonable that I have to manually list the generated header as a source of the library, but from there Meson should be able to figure that anything that depends on the library also depends on the generated header.

@ssssam
Copy link
Contributor Author

ssssam commented Nov 22, 2016

Here's my testcase in full:

meson-mkenums-testcase.zip

Build fails 1st time and succeeds second time (if you run with ninja-build -j 1 it probably succeeds first time)

@rhd
Copy link
Contributor

rhd commented Nov 22, 2016

Does the race condition still happen if you do

lib1 = library('lib1', enums)

@rhd
Copy link
Contributor

rhd commented Nov 22, 2016

btw, when I run your test case, meson/ninja gets stuck in an infinite loop - weird.

rhd@rob-desktop ~/Desktop/meson-mkenums-testcase $ ninja -C build/
ninja: Entering directory `build/'
[0/1] Regenerating build files
The Meson build system
Version: 0.37.0.dev1
Source dir: /home/rhd/Desktop/meson-mkenums-testcase
Build dir: /home/rhd/Desktop/meson-mkenums-testcase/build
Build type: native build
Project name: meson-enums-dependencies
Native c compiler: cc (gcc 5.4.0)
Build machine cpu family: x86_64
Build machine cpu: x86_64
Build targets in project: 4
[0/1] Regenerating build files
The Meson build system
Version: 0.37.0.dev1
Source dir: /home/rhd/Desktop/meson-mkenums-testcase
Build dir: /home/rhd/Desktop/meson-mkenums-testcase/build
Build type: native build
Project name: meson-enums-dependencies
Native c compiler: cc (gcc 5.4.0)
Build machine cpu family: x86_64
Build machine cpu: x86_64
Build targets in project: 4
[0/1] Regenerating build files
The Meson build system
Version: 0.37.0.dev1
Source dir: /home/rhd/Desktop/meson-mkenums-testcase
Build dir: /home/rhd/Desktop/meson-mkenums-testcase/build
Build type: native build
Project name: meson-enums-dependencies
Native c compiler: cc (gcc 5.4.0)
Build machine cpu family: x86_64
Build machine cpu: x86_64
Build targets in project: 4
[0/1] Regenerating build files

@jpakkane
Copy link
Member

Meson should be able to figure that anything that depends on the library also depends on the generated header.

There are two different kinds of generated headers: internal and external. Meson can not tell which one is which. The developer has to specify it explicitly somehow. Adding a dependency on all headers bloats the Ninja file and is a parallelisation hurdle.

@ssssam
Copy link
Contributor Author

ssssam commented Nov 23, 2016

Does the race condition still happen if you do
lib1 = library('lib1', enums)

Yes

There are two different kinds of generated headers: internal and external. Meson can not tell which one is which. The developer has to specify it explicitly somehow. Adding a dependency on all headers bloats the Ninja file and is a parallelisation hurdle.

OK. So I guess the only thing we can do is update the docs for gnome.mkenums() to make it clear that anyone using generated enum headers in a library cannot use link_with: library, but must instead use library_dep = declare_dependency(sources: enum_header, link_with: library) and then dependencies: library_dep. i'd prepare a patch if I had any idea how to do patches against the wiki :-)

@ssssam
Copy link
Contributor Author

ssssam commented Nov 23, 2016

argh, although that solution fixes the testcase, it creates some new error in the Tracker build system:

Meson encountered an error:
Duplicate output 'src/libtracker-common/tracker-enum-types.h' from 'CustomTarget' 'tracker-enum-types.h'; conflicts with 'src/libtracker-common/tracker-enum-types.h' from 'CustomTarget' 'tracker-enum-types.h'

This is kind of cryptic, but my guess is that if the dependency created by declare_dependency() appears twice in the depends: list of some other target we might hit this issue?

@nirbheek
Copy link
Member

The build file makes the issue clear. We build and link in separate invocations. We could do it in one go, but I think we don't because Ninja cannot handle gcc depfiles with multiple outputs. So here's the compile step with all its dependency steps:

build final@exe/main.c.o: c_COMPILER ../main.c

And this is the link step + dependencies:

build enum-types.h: CUSTOM_COMMAND ../enum-types.h.template ../enums.h

build enum-types.c: CUSTOM_COMMAND ../enum-types.c.template ../enums.h | enum-types.h

build lib1@sha/enum-types.c.o: c_COMPILER enum-types.c | enum-types.h

build liblib1.so: c_LINKER lib1@sha/enum-types.c.o

build lib1@sha/liblib1.so.symbols: SHSYM liblib1.so

build final: c_LINKER final@exe/main.c.o | lib1@sha/liblib1.so.symbols

The library dependency is a link_with dependency, so the dependency is only added at the linker step. Meson is doing everything fine fwict. Your executable uses the generated header from the .c file, so it should be added to the list of sources for the executable.

Do you see anything wrong with this assessment?

@nirbheek
Copy link
Member

@ssssam Adding the custom target twice in depends should not cause this. Could you link us to the branch which has your meson build file?

Also, I've added a note about generated sources to the mkenums documentation.

@ssssam
Copy link
Contributor Author

ssssam commented Nov 28, 2016

The branch is here: https://git.gnome.org/browse/tracker/log/?h=wip/sam/meson

I experience the issue with Meson commit ac78ae4

@nirbheek
Copy link
Member

I don't see any tracker-enum-types.h generation in that branch. Is it something that you haven't pushed yet?

@ssssam
Copy link
Contributor Author

ssssam commented Nov 29, 2016 via email

@nirbheek
Copy link
Member

nirbheek commented Dec 1, 2016

I still don't see any gnome.mkenums invocations in the build files. Am I looking in the right place?

https://git.gnome.org/browse/tracker/commit/?h=wip/sam/meson&id=10e8b30c7cab74f29179bae91b82cadb90197761

@ssssam
Copy link
Contributor Author

ssssam commented Dec 1, 2016

We're getting deep into the 21st century but I still can't use Git correctly half the time. Here's the actual commit in which I add enum generation, now actually 100% pushed: https://git.gnome.org/browse/tracker/commit/?h=wip/sam/meson&id=4256465c7f4659d26ed01e2cf8ef561afaa8e910

Sorry for the confusion !

@ssssam
Copy link
Contributor Author

ssssam commented Dec 4, 2016

I've managed to get a successful build (with various changes on top the half-finished branch I pushed there) just by commenting out the 'Duplicate outputs' check. So I guess what's happening is some kind of false positive when the generated enum header required by libtracker-common ends up being a source of the various Vala libraries and programs that link to libtracker-common.

What problem does the 'duplicate outputs' check prevent?

@jpakkane
Copy link
Member

jpakkane commented Dec 4, 2016

If you have multiple steps producing the same output file, then choosing which one to use is undecidable and is probably dealt with similar to undefined behaviour in C. That is: the right thing might happen or anything at all might happen.

@ssssam
Copy link
Contributor Author

ssssam commented Dec 4, 2016

I've managed to distill a testcase: ssssam@6a22ac1

This also has made me realise that it's possible to work around this issue by removing anywhere that a dependency is listed 'redundantly'. For example, if dep_a depends on dep_b, a program that only depends on dep_a will not trigger the issue (and will link against dep_b implicitly), while a program that explicitly depends on dep_a and dep_b will produce this sort of cryptic error:

Duplicate output 'dependency-generated/enum-types.h' from 'CustomTarget' 'enum-types.h'; conflicts with 'dependency-generated/enum-types.h' from 'CustomTarget' 'enum-types.h'

If you have multiple steps producing the same output file, then choosing which one to use is undecidable and is probably dealt with similar to undefined behaviour in C. That is: the right thing might happen or anything at all might happen.

That makes sense. In this case there should be only one step generating enum-types.h, so either gnome.mkenums() has a bug and is generating multiple steps to generate that file, or the 'duplicate outputs' detection code is getting a false positive. Looking at the error message, I suspect the latter. I'm not quite sure how to fix it though.

@jpakkane
Copy link
Member

jpakkane commented Dec 4, 2016

Our transitive dependencies do need a bit more thinking over. This is part of #1057.

geier1993 pushed a commit to geier1993/meson that referenced this issue Dec 29, 2016
The same vapi or vala might be added multiple times. Don't freak out if
that happens. Only freak out if a vapi or vala generated source by the
same name and the output same path is added twice.

This should never happen anyway since we would refuse to create the
target in the first place in theory, but it might happen because of bugs
in generators and custom targets.

Closes mesonbuild#1084
gnomesysadmins pushed a commit to GNOME/mutter that referenced this issue Sep 29, 2020
This is essentially a revert of
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/326. This commit
had the unintended side effect that the built sources are actually
rebuilt for every individual user of libmutter_dep. With there being more
tests, the number of targets to build is getting too high.

Not doing this reduces the number of targets from 2044 to 874, thus
saving man hours and CI burnt cycles in the long run. There's the slight
risk of reintroducing the random build breaks, but mutter is essentially
doing as suggested at mesonbuild/meson#1084
(the only difference being addressed in the previous commit), so it
should work on the meson side.
gnomesysadmins pushed a commit to GNOME/mutter that referenced this issue Sep 30, 2020
This is essentially a revert of
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/326. This commit
had the unintended side effect that the built sources are actually
rebuilt for every individual user of libmutter_dep. With there being more
tests and generated files, the number of targets to build is increasing
squarely.

Not doing this reduces the number of targets from 2044 to 874, thus
saving man hours and CI burnt cycles in the long run. There's the slight
risk of reintroducing the random build breaks, but mutter is essentially
doing as suggested at mesonbuild/meson#1084
(the only difference being addressed in the previous commit), so meson
ought to behave as expected.
gnomesysadmins pushed a commit to GNOME/mutter that referenced this issue Sep 30, 2020
This is essentially a revert of
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/326. This commit
had the unintended side effect that the built sources are actually
rebuilt for every individual user of libmutter_dep. With there being more
tests and generated files, the number of targets to build is increasing
squarely.

Not doing this reduces the number of targets from 2044 to 874, thus
saving man hours and CI burnt cycles in the long run. There's the slight
risk of reintroducing the random build breaks, but mutter is essentially
doing as suggested at mesonbuild/meson#1084
(the only difference being addressed in the previous commit), so meson
ought to behave as expected.

https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1458
gnomesysadmins pushed a commit to GNOME/mutter that referenced this issue Sep 30, 2020
This is essentially a revert of
https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/326. This commit
had the unintended side effect that the built sources are actually
rebuilt for every individual user of libmutter_dep. With there being more
tests and generated files, the number of targets to build is increasing
squarely.

Not doing this reduces the number of targets from 2044 to 874, thus
saving man hours and CI burnt cycles in the long run. There's the slight
risk of reintroducing the random build breaks, but mutter is essentially
doing as suggested at mesonbuild/meson#1084
(the only difference being addressed in the previous commit), so meson
ought to behave as expected.

https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1458
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants