search

mesonbuild / meson

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

support install_symlink() pointing to target #11519

Open pobrn opened 1 year ago

pobrn commented 1 year ago

Currently, the pointing_to argument of install_symlink() only accepts strings:

ERROR: install_symlink keyword argument 'pointing_to' was of type Executable but should have been str

I think it would be useful if it also accepted targets that are installed (probably restricted to targets that have a single output). This would be useful for adding alternative names for executables, libraries, etc. (which was one of the original motivations for adding install_symlink() in the first place - this would make it even simpler and more robust).

For example:

x = executable('x', ...)
install_symlink('y', pointing_to: x)

It would be also nice if in this case the install_dir argument could be omitted, and the install_dir of the pointed-to target used. It not clear to me whether relative or absolute paths should be used, but I imagine that could be selected using a keyword argument.

pobrn commented 1 year ago

Here is a rough attempt:

```diff diff --git a/mesonbuild/backend/backends.py b/mesonbuild/backend/backends.py index c7615b776..73b204db9 100644 --- a/mesonbuild/backend/backends.py +++ b/mesonbuild/backend/backends.py @@ -1787,10 +1787,53 @@ class Backend: links: T.List[build.SymlinkData] = self.build.get_symlinks() for l in links: assert isinstance(l, build.SymlinkData) - install_dir = l.install_dir + import pprint + + if isinstance(l.target, build.Target): + if not l.target.should_install(): + raise MesonException(f'Cannot install symlink "{l.name}" that points to target "{l.target.name}" which will not be installed.') + + outputs = l.target.get_outputs() + if len(outputs) != 1: + raise MesonException(f'Cannot install symlink "{l.name}" that points to target "{l.target.name}" which has multiple outputs.') + + outdirs, install_dir_names, custom_install_dir = l.target.get_install_dir() + pprint.pprint(outdirs) + pprint.pprint(install_dir_names) + pprint.pprint(custom_install_dir) + assert len(outdirs) == 1 # if the target has a single output, surely this must always be true? + + target_filename = self.get_target_filename(l.target) + + if not l.install_dir: + # if no install_dir is specified, install the symlink next to the target + install_dir = outdirs[0] + symlink_target = target_filename + if l.name == symlink_target: + raise MesonException(f'Cannot install symlink "{l.name}" because it will have the same path as its target.') + else: + target_path = os.path.join(outdirs[0], target_filename) + idir = l.install_dir + + if not os.path.isabs(idir): + idir = os.path.join(self.environment.get_prefix(), idir) + + if not os.path.isabs(target_path): + target_path = os.path.join(self.environment.get_prefix(), target_path) + + symlink_target = os.path.relpath(target_path, idir) + install_dir = l.install_dir + else: + symlink_target = l.target + install_dir = l.install_dir + + pprint.pprint(symlink_target) + pprint.pprint(install_dir) + name_abs = os.path.join(install_dir, l.name) + pprint.pprint(name_abs) tag = l.install_tag or self.guess_install_tag(name_abs) - s = InstallSymlinkData(l.target, name_abs, install_dir, l.subproject, tag) + s = InstallSymlinkData(symlink_target, name_abs, install_dir, l.subproject, tag) d.symlinks.append(s) def generate_subdir_install(self, d: InstallData) -> None: diff --git a/mesonbuild/build.py b/mesonbuild/build.py index c0174253a..91c6f9522 100644 --- a/mesonbuild/build.py +++ b/mesonbuild/build.py @@ -2935,10 +2935,10 @@ class Data(HoldableObject): @dataclass(eq=False) class SymlinkData(HoldableObject): - target: str + target: T.Tuple[str, Target] name: str - install_dir: str subproject: str + install_dir: T.Optional[str] = None install_tag: T.Optional[str] = None def __post_init__(self) -> None: diff --git a/mesonbuild/interpreter/interpreter.py b/mesonbuild/interpreter/interpreter.py index 525918683..59f20c284 100644 --- a/mesonbuild/interpreter/interpreter.py +++ b/mesonbuild/interpreter/interpreter.py @@ -2266,8 +2266,8 @@ class Interpreter(InterpreterBase, HoldableObject): @typed_pos_args('symlink_name', str) @typed_kwargs( 'install_symlink', - KwargInfo('pointing_to', str, required=True), - KwargInfo('install_dir', str, required=True), + KwargInfo('pointing_to', (str, build.Target), required=True), + KwargInfo('install_dir', (str, NoneType)), INSTALL_TAG_KW, ) def func_install_symlink(self, node: mparser.BaseNode, @@ -2275,8 +2275,13 @@ class Interpreter(InterpreterBase, HoldableObject): kwargs) -> build.SymlinkData: name = args[0] # Validation while creating the SymlinkData object target = kwargs['pointing_to'] - l = build.SymlinkData(target, name, kwargs['install_dir'], - self.subproject, kwargs['install_tag']) + install_dir = kwargs['install_dir'] + + if not isinstance(target, build.Target) and not install_dir: + raise InvalidArguments('When the symlink target is a string, install_dir is required.') + + l = build.SymlinkData(target, name, self.subproject, + install_dir, kwargs['install_tag']) self.build.symlinks.append(l) return l diff --git a/mesonbuild/modules/gnome.py b/mesonbuild/modules/gnome.py index a0e533059..b67c50bf7 100644 --- a/mesonbuild/modules/gnome.py +++ b/mesonbuild/modules/gnome.py @@ -1329,7 +1329,7 @@ class GnomeModule(ExtensionModule): if symlinks: link_target = os.path.join(os.path.relpath(c_install_dir, start=m_install_dir), m) l_data = build.SymlinkData(link_target, os.path.basename(m), - m_install_dir, state.subproject, install_tag='doc') + state.subproject, m_install_dir, install_tag='doc') else: try: m_file = mesonlib.File.from_source_file(state.environment.source_dir, l_subdir, m) ```
eli-schwartz commented 1 year ago

I think it would be useful if it also accepted targets that are installed

In old discussions about this, that was indeed requested. From recollection, the design of install_symlink was intended as an MVP that everyone could agree on -- a portable way to produce symlink objects when you know what the source and destination should be.

Discussion about handling executables got bogged down a bit by concerns about how to handle the names cross-platform, e.g. should meson automatically add a ".exe" to the symlink name when it's pointing to a Windows executable that gains a .exe extension there? Should it just point "foo" to the "bar.exe" file?

I thought it was a bit of a weak argument, myself, the answer should just be "yes, do the magic"... I'm definitely open to the idea of extending this.

eli-schwartz commented 1 year ago

We should probably convert the executable to a string inside func_install_symlink, though, not do it inside the backend.

pobrn commented 1 year ago

I thought it was a bit of a weak argument, myself, the answer should just be "yes, do the magic"... I'm definitely open to the idea of extending this.

So as far as I understand, the logic would be:

  1. get the given target
  2. check that it has only a single "canonical" output
  3. get a list of things that will be installed due to the given target
  4. for each item in the above list: i. create a symlink whose name is decorated the same way as the the item and points to the item

This would create symbolic links for all sonames of a shared library, implibs, debug files (pdb), etc. But I feel this would overload the behaviour of install_symlink() a bit too much. Maybe an alias_target() would be more appropriate for this.

We should probably convert the executable to a string inside func_install_symlink, though, not do it inside the backend.

I did it there because e.g. get_target_filename() is part of Backend and the methods thereof are not really accessed from Interpreter as far as I can see.

eli-schwartz commented 1 year ago

This would create symbolic links for all sonames of a shared library, implibs, debug files (pdb), etc. But I feel this would overload the behaviour of install_symlink() a bit too much. Maybe an alias_target() would be more appropriate for this.

I think that's not necessary, we can just create a symbolic link to the primary output -- people creating symlinks really just want to be able to access the shared or static library, or executable, under a different name. pdb files, the multiple symlink aliases of a versioned library, etc. are not actually intended to be symlinked.

I did it there because e.g. get_target_filename() is part of Backend

That's an interesting point, and I'm not sure what the best thing to do is, but I will note that doing it in the backend means that if there's an error, you don't get it when you use install_symlink and you don't get a reference to the file and line number that provoked the error -- but instead you get an anonymous error while writing out build.ninja.

xclaesse commented 1 year ago

get_target_filename() and friends being in backend has annoyed me more than once. I think all those functions should be moved as method on the target object.

eli-schwartz commented 1 year ago

Hold on, rereading this, the only point of get_target_filename() is to produce a symlink in the source tree if the executable/whatever you are trying to use as a pointing_to target, is install: false.

The answer to this is simple: no.

No. The install_symlink() function clearly installs something, and must point to something that's installed. This is another reason to do this in the interpreter, not the backend: because get_target_filename should be replaced by "error: you may not do this".

pobrn commented 1 year ago

Hold on, rereading this, the only point of get_target_filename() is to produce a symlink in the source tree if the executable/whatever you are trying to use as a pointing_to target, is install: false.

Is this in reference to the patch I attached? If so, then I don't understand what you mean because whether or not the target is installed is checked. Or am I missing something?

Also, producing a symlink in the build directory would be a good feature in my opinion. For example, at the moment one has to use custom_target() (or something) and install_symlink() if you also need the symlink in the build directory.

eli-schwartz commented 1 year ago

Also, producing a symlink in the build directory would be a good feature in my opinion.

Speaking as a project maintainer I consider this a bad feature.

...

If you need a build-time symlink then instead of using a dedicated "install" function without an install dir, just use a custom_target with the /usr/bin/ln program -- this already works. What's the downside of this?

pobrn commented 1 year ago

If you need a build-time symlink then instead of using a dedicated "install" function without an install directory, just use a custom_target with the /usr/bin/ln program -- this already works. What's the downside of this?

I would want to have both. In order to have a seamless devenv experience, I would argue that the symlinks would need to exist in the build directory and be installed as well. Currently one needs to use both custom_target() and install_symlink() to achieve this, right? Also using custom_target() will make the build dependent on ln.

I am not saying that install_symlink() should be modified. I would just like to see a function that will create a symlink in the build directory and install it as well, and also accepts other (build) targets as (symlink) target.