Allow link_depends to take strings, Files or generated objects. Close… #1651

dcbaker · 2017-04-20T16:27:28Z

Currently only strings can be passed to the link_depends argument of
executable and *library, which solves many cases, but not every one.
This patch allows generated sources and Files to be passed as well.

On the implementation side, it uses a helper method to keep the more
complex logic separated from the init method.

aaronp24 · 2017-04-26T17:11:08Z

This doesn't seem to work for custom targets. I'm using this as my test: aaronp24/libglvnd@809c011

The rules in question are these:

glapi_gen_libopengl_exports_script = find_program('gen_libOpenGL_exports.py')
glapi_gen_gl_core_xml = files('xml/gl.xml')
g_opengl_exports_sym = custom_target('g_opengl_exports.sym',
    command : [glapi_gen_libopengl_exports_script, 'opengl', glapi_gen_gl_core_xml],
    capture : true,
    depend_files : files('genCommon.py', 'gen_libOpenGL_exports.py') + glapi_gen_gl_core_xml,
    output : 'g_opengl_exports.sym',
)
g_opengl_exports_sym_dep = declare_dependency(
    sources : g_opengl_exports_sym,
    link_args : '-Wl,--version-script,@0@'.format(g_opengl_exports_sym.full_path()),
)

opengl = shared_library('OpenGL', 'libopengl.c',
    link_depends : [
        g_opengl_exports_sym,
    ],
    link_whole : [
        glapi_opengl,
    ],
    dependencies : [
        gldispatch_dep,
        g_opengl_exports_sym_dep,
    ],
    include_directories : [inc, util_inc],
    version : '0.0.0',
    install : true,
)

I added some tracing to process_link_depends to print out the entries here, and this thing is a CustomTargetHolder with a CustomTarget that apparently has a get_generated_sources, but where s.get_generated_sources() returns []. So it doesn't complain, but it also doesn't create a dependency on the linker script.

dcbaker · 2017-04-26T17:17:21Z

You're right. get_outputs() returns what we actually want, I think.

aaronp24 · 2017-04-26T17:32:37Z

Still doesn't work. This sticks g_opengl_exports.sym into the dependency list, which somehow turns into /home/aaron/git/libglvnd/src/OpenGL/g_opengl_exports.sym in build.ninja. But the generated file is /tmp/b/src/generate/g_opengl_exports.sym

dcbaker · 2017-04-26T17:35:29Z

yup. I just noticed that. unclean build working, clean build not so much.

I think what I really want to do is add a link_script keyword argument, like the vs_module_defs parameter, which could handle both generating the -Wl,--version-script and the dependency anyway.

dcbaker · 2017-04-26T18:27:23Z

@aaronp24 I think this version should work for you. I'll see about adding an extra kwarg to make this even easier.

aaronp24 · 2017-04-26T18:59:13Z

Nope. That creates a dependency on src/OpenGL/g_opengl_exports.sym, but the generated file is in src/generate/g_opengl_exports.sym:

$ ninja -C /tmp/b src/generate/g_opengl_exports.sym
ninja: Entering directory `/tmp/b'
[1/1] 'Generating g_opengl_exports.sym with a meson_exe.py custom command.'
$ ninja -C /tmp/b
ninja: Entering directory `/tmp/b'
ninja: error: 'src/OpenGL/g_opengl_exports.sym', needed by 'src/OpenGL/libOpenGL.so.0.0.0', missing and no known rule to make it

I could probably work around that somehow, by moving files around, I guess, but it seems confusing.

dcbaker · 2017-04-26T19:13:32Z

No, this should happen transparently. I'll poke at it some more after lunch.

dcbaker · 2017-04-26T20:46:37Z

Okay, it was a really trivial bug on my part. I've tested glvnd with this version of the patch, and it works (I do have a very small glvnd patch.

aaronp24 · 2017-04-26T21:39:09Z

Thanks, that seems to work!

The libglvnd thing is just an experiment in progress, so I'm definitely open to suggestions.

dcbaker · 2017-04-26T21:40:46Z

I figured. I'm trying to port mesa ATM, so having GLVND ported to meson too would be pretty nifty.

jpakkane · 2017-04-26T21:44:24Z

This message is posted to every outstanding merge request.

We have transitioned from Github wiki to our new documentation system that generates all documentation directly from the Git repo. This means that from now on every merge request must come with corresponding documentation changes.

If your MR requires documentation changes, do the necessary changes to files in docs/markdown.

If your MR adds a new feature, add a description to the release notes in docs/markdown/Release-notes-for-0.41.0.md.

If this commit is just a bugfix with no functional changes, you don't need to do anything.

Thank you

jpakkane · 2017-04-30T23:16:55Z

Traditionally Meson's implementation has been a mix of File objects and strings. We are trying to move closer to the case where all entries that are files (whether source or generated) are only stored in File objects. This means converting strings to Files as soon as possible and only going back to strings when serializing in the backend. See for example #1706. Having dependencies as strings rather than objects makes many things needlessly complicated (a fair amount of the terribleness of ninjabackend.py is caused by this).

process_link_depends currently seems to convert file objects to strings when it should be doing the opposite.

ignatenkobrain · 2017-05-01T18:23:27Z

mesonbuild/build.py

+            elif isinstance(s, str):
+                self.link_depends.append(
+                    File.from_source_file(environment.source_dir, self.subdir, s))
+                    # os.path.join(environment.source_dir, self.subdir, s))


[flake8]

[E116] unexpected indentation (comment)

ignatenkobrain · 2017-05-01T18:23:27Z

mesonbuild/build.py

+            elif hasattr(s, 'get_outputs'):
+                self.link_depends.extend(
+                    [File.from_built_file(s.subdir, p) for p in s.get_outputs()])
+                    # [os.path.join(s.subdir, p) for p in s.get_outputs()])


[flake8]

[E116] unexpected indentation (comment)

dcbaker · 2017-05-01T18:37:48Z

Updated link_depends to always be a File instead of a string. It does require some changes to the ninja backend (and possible the VS backends, but I haven't seen those come back yet)

dcbaker · 2017-05-02T22:54:16Z

No VS backend changes appear to be necessary. Anything else I should change, or is this ready to merge?

jpakkane · 2017-05-03T15:30:43Z

mesonbuild/backend/ninjabackend.py

+                return t.relative_name()
+            else:
+                return t.absolute_path(self.environment.get_source_dir(),
+                                       self.environment.get_build_dir())


Add an else that throws unreachable code exception here.

to the if t.isbuilt if, or the outer one?

The outer one. If there is an item that is neither of the tested things, it is silently discarded. That is not good.

It gets passed to self.get_target_filename() if it isn't handled in that if block, which is what it did before my patch. Is that not correct?

My bad, got cross-eyed.

No worries, happens to me all the time.

jpakkane · 2017-05-03T15:31:17Z

mesonbuild/build.py

+
+        This is designed to handle strings, Files, and the output of Custom
+        Targets. Notably it doesn't handle generator() returned objects. Since
+        adding them as a link depends would inherently cuase them to be


cuase -> cause

…mesonbuild#1172 Currently only strings can be passed to the link_depends argument of executable and *library, which solves many cases, but not every one. This patch allows generated sources and Files to be passed as well. On the implementation side, it uses a helper method to keep the more complex logic separated from the __init__ method. This also requires that Targets set their link_depends paths as Files, and the backend is responsible for converting to strings when it wants them. This adds tests for the following cases: - Using a file in a subdir - Using a configure_file as an input - Using a custom_target as an input It does not support using a generator as an input, since currently that would require calling the generator twice, once for the -Wl argument, and once for the link_depends. Also updates the docs.

dcbaker · 2017-05-04T21:17:04Z

Is there anything missing for this to land?

jon-turney · 2017-05-16T10:44:09Z

No VS backend changes appear to be necessary.

This seems to be because VS backend has always been ignoring link_depends. Of course, now that may contain generated files, this is a problem, see #1799

dcbaker force-pushed the generated-link-depends branch from cca6c94 to b6d180a Compare April 26, 2017 17:25

dcbaker force-pushed the generated-link-depends branch from b6d180a to 8ee972d Compare April 26, 2017 18:26

dcbaker force-pushed the generated-link-depends branch 2 times, most recently from f1a544d to e648ead Compare April 26, 2017 18:50

dcbaker force-pushed the generated-link-depends branch from e648ead to cadcff3 Compare April 26, 2017 20:45

aaronp24 added a commit to aaronp24/libglvnd that referenced this pull request Apr 27, 2017

Clean up generation rules now that mesonbuild/meson#1651 works

0ab5703

dcbaker force-pushed the generated-link-depends branch from cadcff3 to 72fb966 Compare May 1, 2017 18:23

ignatenkobrain reviewed May 1, 2017

View reviewed changes

dcbaker force-pushed the generated-link-depends branch from 72fb966 to 6309ffb Compare May 1, 2017 18:34

jpakkane requested changes May 3, 2017

View reviewed changes

dcbaker force-pushed the generated-link-depends branch 2 times, most recently from 2d4484f to fb251bf Compare May 3, 2017 17:14

dcbaker force-pushed the generated-link-depends branch from fb251bf to dd648d4 Compare May 4, 2017 21:16

jpakkane merged commit 7053d9a into mesonbuild:master May 8, 2017

dcbaker deleted the generated-link-depends branch May 8, 2017 20:04

jon-turney mentioned this pull request May 16, 2017

VS and Xcode backends ignore link_depends #1799

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow link_depends to take strings, Files or generated objects. Close… #1651

Allow link_depends to take strings, Files or generated objects. Close… #1651

dcbaker commented Apr 20, 2017

aaronp24 commented Apr 26, 2017

dcbaker commented Apr 26, 2017 •

edited

Loading

aaronp24 commented Apr 26, 2017

dcbaker commented Apr 26, 2017

dcbaker commented Apr 26, 2017

aaronp24 commented Apr 26, 2017

dcbaker commented Apr 26, 2017

dcbaker commented Apr 26, 2017

aaronp24 commented Apr 26, 2017

dcbaker commented Apr 26, 2017

jpakkane commented Apr 26, 2017

jpakkane commented Apr 30, 2017

ignatenkobrain May 1, 2017

ignatenkobrain May 1, 2017

dcbaker commented May 1, 2017

dcbaker commented May 2, 2017

jpakkane May 3, 2017

dcbaker May 3, 2017 •

edited

Loading

jpakkane May 3, 2017

dcbaker May 3, 2017

jpakkane May 3, 2017

dcbaker May 3, 2017

jpakkane May 3, 2017

dcbaker commented May 4, 2017

jon-turney commented May 16, 2017

Allow link_depends to take strings, Files or generated objects. Close… #1651

Allow link_depends to take strings, Files or generated objects. Close… #1651

Conversation

dcbaker commented Apr 20, 2017

aaronp24 commented Apr 26, 2017

dcbaker commented Apr 26, 2017 • edited Loading

aaronp24 commented Apr 26, 2017

dcbaker commented Apr 26, 2017

dcbaker commented Apr 26, 2017

aaronp24 commented Apr 26, 2017

dcbaker commented Apr 26, 2017

dcbaker commented Apr 26, 2017

aaronp24 commented Apr 26, 2017

dcbaker commented Apr 26, 2017

jpakkane commented Apr 26, 2017

jpakkane commented Apr 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker commented May 1, 2017

dcbaker commented May 2, 2017

Choose a reason for hiding this comment

dcbaker May 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbaker commented May 4, 2017

jon-turney commented May 16, 2017

dcbaker commented Apr 26, 2017 •

edited

Loading

dcbaker May 3, 2017 •

edited

Loading