Add full_path() method to files objects

[tristan957]

I am missing some places where mesonlib.File is probably assumed. This is a work in progress, but it worked in my very minimal test case.

The holder pattern is quite nice. Wasn't super hard to contribute this at all.

Fixes #8433

[tristan957]

Besides failing tests, known missing parts of PR:

• FeatureNew decorators
• snippets contribution
• docs update for new method and new type since I don't think file object are currently documented

[jpakkane]

This should probably be absolute_path instead as it is unambiguous. It is also what the Python function is called, probably for this exact reason.

But more importantly the documentation should be upfront and explicit about the fact that this is a very dangerous thing to do. Strings do not carry dependency information so if you, for example, pass the result to some target, Meson will not know of the dependency between the two and neither will Ninja. This can easily lead to broken builds. The right thing to do in almost all cases is to put the file object directly in the command argument array and Meson will do the right thing. Unfortunately based on experience people will not do that as they assume it "would not work".

[tristan957]

@jpakkane thanks for your input. I think you are definitely correct about the documentation being super explicit about this method having very specific use-cases.

[xclaesse]

This should probably be absolute_path instead as it is unambiguous. It is also what the Python function is called, probably for this exact reason.

I understand your concern but unfortunately all other meson objects already have .full_path() (e.g. BuildTarget, CustomTarget, ExternalProgram). I think in this case consistency is better, or we should deprecate them all and replace by .absolute_path() but IMHO that would annoy all our users for little gain.

[xclaesse]

@tristan957 I think this is the wrong approach, why not add support for File object in message() instead? Wrapping files into a FileHolder, while making general sense, is going to cause countless regressions because we have isinstance() all over the place.

[jpakkane]

If that is the only thing this would be used for, then definitely.

[tristan957]

I was thinking that it could make sense to model files as FileHolders from an interpreter perspective, and this could be a good PR to possibly match how other interpreter object are also modeled as Holders.

I am honestly struggling to come up with use cases for this method, and people who care about this should propose their use cases. I thought I had one while redoing the build system on my project, but I seem to have gotten away without needing it.

Re full_path() vs absolute_path(): I think coming up with internal consistency should be prio #1. Once that is done, if absolute_path() is the way to go, then I don't think the churn is that big a deal because Meson is extremely consistent in its usage of full_path(), so people could just s/full_path/absolute_path/ in a single commit.

[xclaesse]

FileHolder makes a lot of sense, yes. But the question is whether it's worth the pain. The only use case I see so far can trivially be done without it.

[tristan957]

I am just going to put the PR on hold until people come up with good use cases.

[jpakkane]

I think this is the wrong approach, why not add support for File object in message() instead?

If this is something you need, then it should be added regardless of what happens to this PR. I

[tristan957]

That is not something that I need currently. If other people do, then I can add it.

[jpakkane]

What did you need this for, then?

In Meson we purposefully avoid adding features just because they seem useful. Every feature must have an actual use case or requirement behind it.

[tristan957]

There was an issue for it and I wanted to fix it for other people.

[randy408]

I have a project where files can be reloaded at runtime when debugging, the files are also part of the build so it just makes sense to generate the asset/source<->fullpath associations from meson for change monitoring and reloading.

[tristan957]

That seems like a reasonable request pending maintainer approval.

@jpakkane

[xclaesse]

Unfortunately it is not that easy, there are many places where we do isinstance(f, mesonlib.File), and they won't work any more because the type is now FileHolder.

[xclaesse]

So basically, in every API that takes file objects you now have to unholder() them first in interpreter.

[tristan957]

@xclaesse I am willing to put in the work to create FileHolder if people think it is an improvement in the interpreter code.

[jpakkane]

interpreterbase.py has a method_call method that deals with method calls to non-interpreterobjects.

[jpakkane]

This should have a warning of something like: "Use this method with caution. The return value is a string and carries no dependency information. Thus if you use it as an argument to a command, Meson does not know that it refers to an actual file and can not set up dependencies properly. Whenever possible you should use the file object directly in the command line. It will be autoexpanded to point to the file and all necessary dependencies are set up automatically."

[gerion0]

There was an issue for it and I wanted to fix it for other people.

Thank you for the effort!

A small example that I need that function for currently:
I have a resource file (res.txt) and a script that uses that file as part of a test case (test.py).
Both files are not in the same folder. I want to communicate the path of res.txt via an environment variable to test.py (by argument is not simply possible due to various reasons).

This results in these meson files:
somewhere/meson.build:

resource_file = files('res.txt')[0]

elsewhere/meson.build:

py_mod = import('python')
py_inst = py_mod.find_installation('python3')

# does not work currently
res_file = 'RESOURCE_FILE=' + resource_file.full_path()

test('python-test',
     py3_inst,
     args: [files('test.py')],
     env: [res_file]
)

Note, that I was not fully able to find a workaround. This here is probably the best:

res_file = 'RESOURCE_FILE=@0@/@1@'.format(meson.source_root(), resource_file)

Putting a file into the format function seems to result in the path relative to the source_root but I'm not sure at all about this. How it is in subprojects?
Also, using / as a path separator is not OS agnostic (although I'm not sure if using the environment is possible on all systems).

[tristan957]

You can set the workdir for the test. Does that help you?

[gerion0]

Hmm, nothing other in the script depends on the working directory so it would be a possible workaround, yes.
However, I think that a full_path() function is cleaner since it gives Meson the full control over the file location.

[gerion0]

BTW, in the above code the dependency to the resource file is gone for the test target (like @jpakkane already mentioned). It does not seem simple, though, to add it back. At least, the depends kwarg of test() only take targets, not files.

[anarazel]

@jpakkane wrote:

What did you need this for, then?

In Meson we purposefully avoid adding features just because they seem useful. Every feature must have an actual use case or requirement behind it.

@tristan957 wrote:

There was an issue for it and I wanted to fix it for other people.

FWIW, I've needed this in postgres' meson conversion, for an error check during configure. We need to make sure that there are no in-tree files left over from configure, as they'll sometimes get "preferred" (due to #include "path" always looking first in the source file's directory). I got away with '@0@'.format(configure_file_return), but that's just this PR in worse. Before the check there were repeated issues due to outdated files being left over and at some point later failing in weird ways.

[tristan957]

I am happy to work on this again if it would be accepted by @jpakkane. Don't want to put in effort for it to languish again.

[bruchar1]

Fixed by #12408.