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

Allow configure_file, custom_target and generator output to subdirectories under build dir #2613

Closed
wants to merge 7 commits into from
Closed

Allow configure_file, custom_target and generator output to subdirectories under build dir #2613

wants to merge 7 commits into from

Conversation

jasonszang
Copy link

@jasonszang jasonszang commented Nov 12, 2017

This patch improve flexibility in source code generation by allowing these three source generating functions output to subdirectories of the current build dir. This is needed for protobuf and other generators / languages that assume generated code to be a subtree instead of a flat directory.

It also allows configuring headers like config.h and version.h into a project specific subdir and then #include <your_prg/config.h>. No longer needed to config and install foo-config.h directly into /usr/local/include.

This fixes #2320. And hopefully #2539, or at least should make things closer.

This should allow correct and efficient compilation of protobuf and other
source generators that assumes its output is a directory tree instead of
a flatten dir.
Should also import Java source generation since we can now write java
code to subdirectories according to its package.
Also add a @RootName@ substitution which is replaced by full input path name with extensions removed.
Tests added:
- unittest for @RootName@ substitution, configure_file
- test case for configure_file into subdirectories
- replace over-simplified protobuf test case with a complex one
  to test custom_target and generator that outputs to subdirs
@jpakkane
Copy link
Member

No! The layout of the build directory is not something users should have control over. It is fully chosen by the backend and different backends do things differently. We can not allow people to throw things in the build dir at random because that breaks things in subtle ways.

If there are problems with some tools or frameworks, then we need to come up with proper solutions for those.

…utputting to subdir is now allowed with this patch.

Test "output escaping" make sure people cannot make their output escape by adding '..' in output path name.
@jasonszang
Copy link
Author

jasonszang commented Nov 12, 2017

The problem is that even though perhaps people should not be given the control over the entire build directory layout (e.g. the object files), they should and need to have control to at least their generated sources, or all kinds of problems will occur. The problem about having to install a foo-config.h directly into /usr/local/include (which is quite ugly, in my opinion) instead of properly generating a project/config.h in a subdir is just one of them. And we have problems with protobuf and generated java sources and all kinds of things that doesn't go well with the flat directory assumption which is a bit over simplified.
Besides, the moment you allowed people invoke custom commands with custom_target or anything else from meson.build, you already allowed people to throw things in the build dir at random. Because their custom command can do anything from writing a/b.c to rm -rf / and it is not the build system's responsibility to stop them from doing UBs (and stop them from build existing projects when they clearly know what they are doing). And, even with the current restriction I can still invoke protoc and generate a source tree in the build dir, or some subdirectories deeper into it. It's just that meson cannot properly use the output. The risk, was there, and is always there. We are just abandoning the benefits for nothing.
And if you really don't like the idea of giving people at least some control over build directory then I suggest we make up a new directory to store the generated sources, put it on the include path, and give people control over that. This will also fix our protoc and java problems. I look forward to hearing your opinions.

@jpakkane
Copy link
Member

In what way does the Protobuf compiler require a directory tree for its output? I tested this and the generated code has include statements that look like the following:

#include "defs.pb.h"

@jasonszang
Copy link
Author

jasonszang commented Nov 12, 2017

Protobuf definitions themselves may have directory structures, which is quite a common practice. For example, you can have

proto/foo.proto
proto/subdir/bar.proto

and in foo.proto you import "subdir/bar.proto".
For this kind of things to work, protocs must be invoked like this:

protoc -I proto --cpp_out outdir proto/foo.proto
protoc -I proto --cpp_out outdir proto/subdir/bar.proto

This will generate a source subtree that mirrors the proto tree structure. And in the generated foo.pb.h, it will #include "subdir/bar.pb.h". Putting all generated sources in the same directory will not work. And using meson subdir will not work too, because protoc need to have the correct proto root or the generated code will be incorrect. See my new tests for an example.
Your single file defs.proto test does not require a directory of course. But it is over-simplified and our existing projects and other real-world projects that use proto (like Tensorflow) do not fall in this category.

raise InvalidArguments('"outputs" must not contain a directory separator.')
if '@BASENAME@' not in rule and '@PLAINNAME@' not in rule and '@ROOTNAME@' not in rule:
raise InvalidArguments('Every element of "output" must contain @BASENAME@, @PLAINNAME@ or @ROOTNAME@.')
if '..' in path_norm_split(rule):
Copy link
Member

Choose a reason for hiding this comment

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

You should just use pathlib.PurePath and then check .parts for ...

…ng --layout flat builds.

Note that currently building complex protobuf with --layout flat still WON'T work,
since @CURRENT_SOURCE_DIR@ will be incorrectly substituted with '../meson-out',
which is obviously not our source dir.
Backend.get_target_source_dir seems a little buggy right now. It seems not taking
layout --flat into consideration. Maybe I should also try to fix it?
@jpakkane
Copy link
Member

I created a bug report on Protobuf to support exporting a flat layout, which seems like the correct solution. However that might not get fixed soon (or possibly ever :( ) so we should probably come up with some interim solution.

First of all protobuf seems such a complicated thing that we should probably create a proper module for it so people can just do:

pb_md = module('protobuf')
pb_sources = pb_mod.generate_sources('foo.proto', 'bar.proto', include_directories : include_directories('someplace'))

rather than playing around with generators and the like.

Secondly, even though we won't let people write randomly to the build dir, we can do that inside the target private directory.

@jasonszang
Copy link
Author

jasonszang commented Nov 12, 2017

To be honest, making Protobuf (and perhaps other source generators like aidl in #2539) change their way of generating codes to just fit meson the build system does not seem to be the correct solution to me. There are too much existing codes out there (and inside google perhaps) which may not fit into this change. After all, build systems are there to provide people tools to build things, not to teach people how to do things correctly or make them cope with the build system. It should be more important to first allow people to build these existing projects up, isn't it? And I really do not feel we can ever push all these IDLs , code generators and custom commands to change their way just for one of our design choices.

As for the target private directory approach, I still feel if we really hate giving away a little control over the build dir so much we should fix it by inventing something like <builddir>/meson-gen, put all generated sources with these three functions (which involves custom commands) inside it, and give people control over its layout. This should be better than generating separately into target private directory. One of the reason is compilation speed. Compiling .pb.cc is slow, especially when the proto defs are complex (Ours are usually complex :( ), and compiling protos into a static library then link it everywhere can save a lot of time. This is especially true if we want to compile a lot of unittest binaries. Compiling a proto tree for tens of times is just way too slow.

And for the proto module approach, I appreciate that, though I still would like a clean solution to install a myprg/config.h instead of myprg-config.h, and perhaps helping the aidl guy in #2539.

@jpakkane
Copy link
Member

compiling protos into a static library then link it everywhere can save a lot of time

You can already do this with, roughly:

pbfiles = generator.generate('foo.proto')
pblib = static_library('protostuff', pbfiles)

and then using pblib. Hmmm, thinking more about it, there might be some dependency issue if you need to use the pb headers directly.

I still would like a clean solution to install a myprg/config.h

You really should not install a config.h in any system wide directory. It is actually forbidden by distros because it has the potential to break absolutely everything (and has done so in the past). Every installed header should have a unique name or at the very least not a super common name like config.h.

@jasonszang
Copy link
Author

jasonszang commented Nov 12, 2017

Hmmm, thinking more about it, there might be some dependency issue if you need to use the pb headers directly.

Yes, pb headers are almost always used directly in application code, and occasionly installed as public headers for other projects.

You really should not install a config.h in any system wide directory.

The boost guys install a ton of config.hpps and quite a few projects are installing config.h and version.h, under a subdirectory of /usr/local/include, of course.
But that is not my point. My point is that I really do not want to install any header directly under /usr/local/include and would like to get all of my project headers into a project folder, be it /usr/local/include/myprj/foo.h or some config things. This seems cleaner and safer to me since two projects may have some random header which happen to have the same name and become incompatible if we install headers directly at the top level. So we came to the choice of getting every header file under project dir and installing nothing at the top level, and having a xxx-config.h getting out seems a little weird.

@jasonszang
Copy link
Author

jasonszang commented Nov 13, 2017

Travis issues seems to be the compatibility between protobuf 3.0.0 and meson's unity build mode, since the error is a compilation one involving redefinition of protobuf internal functions. The issue emerged because I replaced the single-file test case with a complex one that involved some seven or more .protos getting compiled and linked together. I'm unable to reproduce this error with protobuf 3.4.1. Perhaps we should consider updating the protobuf version used by Travis?

@liugang
Copy link
Contributor

liugang commented Nov 13, 2017

I agree with jasonszang, not every projects or gentools need or support a flat layout, everyone like creating a subdir to hold some files with same name, rather than give different name in a same directory.

@jasonszang
Copy link
Author

jasonszang commented Nov 17, 2017

Are there any more thoughts on this pr, or other solutions to our protobuf problem (that everyone will bump on when they begin to use complex protos)? Since it looks like that the protobuf guys are not quite keen to adopt a flat output layout.

@jasonszang jasonszang closed this Nov 28, 2017
@jasonszang
Copy link
Author

Closed because meson does not satisfy our requirements of building complex proto based projects and there seem to be zero chance to change that in foreseeable future.

@nirbheek
Copy link
Member

Fwiw, something similar is also needed by Cargo in #2617, and I have a smaller commit which does the job for that use-case: b896a3c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow configure_file, custom_target and generator output file to subdirectories under builddir
4 participants