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

Remove include_directories absolute path validation #12281

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

GertyP
Copy link
Contributor

@GertyP GertyP commented Sep 21, 2023

This was a draft that started out as a new option to 'allow_abs_paths' but since there were objections to that approach, and since there's also been no objections yet to the suggestion of removing the overly simplistic check that prevents quite a nice, clean, and still safe and valid style of include_directories use, this PR has now evolved in to the following, which I thought should be reflected in an updated title and less rambly description -

Removed include_directories absolute path validation

The 'Tried to form an absolute path to a dir in the source tree' check
is a blunt instrument in that it's too simplistic to correctly identify
problematic, non-project-relative absolute path use without preventing
perfectly good and safe absolute include path use that makes for very
clean, maintainable/refactorable build files.

Guesing at potential problems and imposing a particular style on meson
users, particularly when those problems don't exist in practice, seems
like something best avoided. The true arbiter of a problem path is
ultimately whether a path actually exists/works; if they work, don't
add unnecessary obstacles for the user; if they don't work, the problem
and solution should be pretty clear to the user.

The original draft description is kept below -


I brought up a discussion about a recent issue I came across, regarding an enforced ban on use of absolute include paths, which I thought might be just a little too blunt and unnecessary an obstacle (in my use case) ... Either that or I misunderstood why this restriction (which isn't well explained in the error message) is being enforced. It didn't get any takers, which I take to mean it was -

  • too long and rambling
  • or had some valid points but weren't significant enough to prompt any agreement or dissent

...or mabye both.

If I have a valid point, then I'm hoping this simple draft PR illustrates one of the solutions that allows me an option to improve the situation for my use-case -

The existing error upon detecting use of an absolute include directory path (within the source tree) is a well-intentioned but blunt tool that unnecessarily prevents use of valid and safe absolute include dir paths.

Absolute paths can be constructed from build-relative absolute paths (e.g. src_root = project_source_root(), ... src_root / 'dir/inc') and so use consistent 'src_root'-relative path construction across multiple meson.build files while also making each meson.build file easy to move without breaking through use of pure relative paths.

Discussed with more context and rationale here -
#12244

Setting the option allow_abs_paths=true prevents this error check and is obviously used at your own risk, while also ensuring the user understands the rationale.

(I think this would also require me to add a little more documentation and a new test case, which I'm happy to do if this is deemed ok)

... Alternatively, hopefully someone can better explain why, even in my use-case, there needs to be a total ban on absolute include directories. Then we might be able to better explain it in the current message.

@GertyP GertyP force-pushed the allow_abs_paths branch 2 times, most recently from 62c1d71 to eb02a6d Compare September 26, 2023 16:20
@eli-schwartz
Copy link
Member

It didn't get any takers, which I take to mean it was -

  • too long and rambling
  • or had some valid points but weren't significant enough to prompt any agreement or dissent
    ...or mabye both.

Perhaps, but in my case the solution is simpler: I missed that notification and didn't see your question. Sorry. :( I've replied with a couple of preliminary thoughts but I haven't got any firm ideas about what I think here, yet.

...

I can tell one thing already, though.

  • Maybe the answer is to simply allow this use case, if we think it's alright for people to do it in some cases.
  • Maybe the answer is to reject the use case.

But I'm pretty sure the answer is not to add a command line option that only serves to tell meson to be less stringent in its opinionatedness. There's a UX complexity tax on command line options, but also in general I think if we want to allow it we should simply allow it. And if we don't want to allow it, we shouldn't allow people to say "okay but please allow it just for me".

@GertyP
Copy link
Contributor Author

GertyP commented Sep 26, 2023

I went ahead and added to the 'absolute paths' error message with what I thought was an explanation of its intention, which has been missing (please correct me if my new addition to the error message is wrong).

I also added a test case to illustrate the use of the proposed new allow_abs_paths option.

Just as I'm writing this, I see @eli-schwartz has responded to the discussion, with an alternative sort of compromise approach which is that each of the many 'meson.build' files in various subdirectories maintains its own strictly relative path variable at the top of the file (e.g. src_root = '../..'), then paths continue to be constructed relative to this variable and so will stay relative. So refactoring meson build files then just requires fixing up the local src_root = in affected instances.

Short of these proposed changes, I think that's a reasonable compromise that works with current meson.

Even though I think I'd still prefer these proposed changes, simply due to there being slightly less to author and maintain in the build files, I'd totally understand the reluctance to take changes that aren't strictly necessary.

@GertyP
Copy link
Contributor Author

GertyP commented Sep 26, 2023

if we want to allow it we should simply allow it. And if we don't want to allow it, we shouldn't allow people to say "okay but please allow it just for me".

I see. It is a similar strategy/tool that I've used elsewhere in similar circumstances: The default takes a strict position which we anticipate can actually be helpful in guiding better use in some/many cases of misuse but, because it can be a bit blunt and where a more nuanced, precise check is impractical, and we recognise some cases where it makes the user's life a bit more difficult than strictly necessary, we allow the user to essentially sign a little disclaimer: "I promise I know what I'm doing with regards to X/Y/Z so just let it go, will you?"

I personally don't mind the addition to the option burden/bloat so, for whatever its worth, my preferences would be -

  1. Take the new option 😄 : Still having the ability to point out uses of potentially problematic, absolute paths by default, but offering a way out for those who recognise their use is fine.
  2. Get rid of the check entirely. Those who run in to problems from relocation-fragile absolute path use will soon see the problem and the solution.
  3. No change: I (and others) can work around the impact of the existing blunt check through the addition of the zstd-like approach you've mentioned... assuming this approach occurs to them; they may just resort to manually fixing up lots of individual broken paths whenever restructuring affects any 'meson.build' files.

@jpakkane
Copy link
Member

jpakkane commented Oct 1, 2023

Adding options should be a last ditch effort and we actively work against adding them. It is always better to fix the underlying problem instead and make things work automatically, even if it takes more implementation effort.

@GertyP
Copy link
Contributor Author

GertyP commented Oct 2, 2023

I don't object to that principle, but that leaves us with either -

  1. Improving the current blunt check that complains about perfectly valid and relocation-safe absolute include paths (that have been quite reasonably composed from project/build-relative absolute path variables): This would require something like carrying around the fundamental meson path components through to the point at which this absolute path check is performed or, when evaluating a path expression early on, storing an internal flag in the result that says whether the final path is constructed from relative paths or, if not, a relocation-safe absolute path variable (like project_source_root() or similar).
  2. Getting rid of the check: Let users with perfectly valid absolute include paths build without tripping them up and forcing them to unnecessarily restructure their build script while also letting users with potentially problematic absolute include paths continue without obstruction if those paths actually work, and if they do run in to problems (e.g. somehow transplant everything onto a different drive and there's a hard-coded absolute path somewhere) then it'll be immediately obvious what the problem and solution is.
  3. Improving the error message so the user better understands exactly what problem is being reported (it really isn't clear from the error message, when I know my use makes for safe, low-maintenance, clear build scripts), and that it might still not actually apply to them in practice but that we're still requiring them to refactor things to appease this check, and preferably a hint at what a reasonable solution is (e.g. having to maintain a relative path variable in each subdir'd meson.build file, zstd-style). Without such a hint, there's a reasonable risk that the author's quite heavily burdened with resorting to do a lot of annoying re-pathing if/when they restructure/subdir-ify their build scripts when they would otherwise have been immune to such breakage with safely constructed abolute paths.

My preference would be #2: It's the least amount of work, it removes some meson check code (and removes the need for a test, I think), it feels as though it has the least impact/burden on users, and it would allow me to continue to use my preferred, problem-free, relocation-safe absolute path construction style that has zero maintenance burden when moving/creating/refactoring 'meson.build' subdirectories.

GertyP added a commit to GertyP/meson that referenced this pull request Oct 9, 2023
The 'Tried to form an absolute path to a dir in the source tree' check
is a blunt instrument in that it's too simplistic to correctly identify
problematic, non-project-relative absolute path use without preventing
perfectly good and safe absolute include path use that makes for very
clean, maintainable/refactorable build files.

Guesing at potential problems and imposing a particular style on meson
users, particularly when those problems don't exist in practice, seems
like something best avoided.  The true arbiter of a problem path is
ultimately whether a path actually exists/works;  if they work, don't
add unnecessary obstacles for the user; if they don't work, the problem
and solution should be pretty clear to the user.

Discussion on this issue -
mesonbuild#12244
mesonbuild#12281
GertyP added a commit to GertyP/meson that referenced this pull request Oct 9, 2023
The 'Tried to form an absolute path to a dir in the source tree' check
is a blunt instrument in that it's too simplistic to correctly identify
problematic, non-project-relative absolute path use without preventing
perfectly good and safe absolute include path use that makes for very
clean, maintainable/refactorable build files.

Guesing at potential problems and imposing a particular style on meson
users, particularly when those problems don't exist in practice, seems
like something best avoided.  The true arbiter of a problem path is
ultimately whether a path actually exists/works;  if they work, don't
add unnecessary obstacles for the user; if they don't work, the problem
and solution should be pretty clear to the user.

Discussion on this issue -
mesonbuild#12244
mesonbuild#12281
@GertyP
Copy link
Contributor Author

GertyP commented Oct 9, 2023

Since this is still a WIP and there's opposition to a new option, but nothing decisive on any of the other suggested possible improvements numbered in my comment above, I thought I'd chuck in a new commit ('Removed include_directories absolute path validation') to illustrate my next preference to this situation, just to see if it gets any traction or outright opposition that would help either move this on or just shut it down.

Failing that, I think the least we should do with this situation is improve the error message so that those in the future, who are prevented from building otherwise valid and safe builds, can at least understand what problems this blunt check is trying to help the user avoid, and that it may be giving false positives that, nevertheless, the user must work-around. So far, and in the linked discussion , I've mentioned what I guess this check is really trying to deal with (i.e. avoiding build breakage from relocating source and build directories) but we shouldn't really have to guess; it would be good to get confirmation and a better message, otherwise some people will inevitably be left scratching their heads from this error message, wondering exactly what's the problem with their perfectly decent absolute include path use.

@eli-schwartz
Copy link
Member

Mechanically, it would be possible to implement option 1 -- there are a couple of other cases of "magic tracking strings" in https://github.com/mesonbuild/meson/blob/master/mesonbuild/interpreter/primitives/string.py which allow doing instance checks in any API that takes a string:

if isinstance(mystring, MesonSourceRootString):
    pass
else:
    do_error('mystring was a str type but was not a tracking string indicating it came '
             'from meson.*source_root() functions, and this is bad because REASON')

Again, I'm relatively neutral on which of the three options to go for (if any). I'm mentioning this because:

  • it is one of the options you enumerated
  • it is a tricky topic and it may not be obvious how to implement it, if one wishes to implement it

so I figured I'd talk about implementation approaches.

@dcbaker
Copy link
Member

dcbaker commented Oct 9, 2023

I am concerned this is going to conflict with #12258, and if I'm correct I don't see a good way to fix it.

GertyP added a commit to GertyP/meson that referenced this pull request Oct 22, 2023
The 'Tried to form an absolute path to a dir in the source tree' check
is a blunt instrument in that it's too simplistic to correctly identify
problematic, non-project-relative absolute path use without preventing
perfectly good and safe absolute include path use that makes for very
clean, maintainable/refactorable build files.

Guesing at potential problems and imposing a particular style on meson
users, particularly when those problems don't exist in practice, seems
like something best avoided.  The true arbiter of a problem path is
ultimately whether a path actually exists/works;  if they work, don't
add unnecessary obstacles for the user; if they don't work, the problem
and solution should be pretty clear to the user.

Discussion on this issue -
mesonbuild#12244
mesonbuild#12281
@GertyP GertyP changed the title Add allow_abs_paths option Remove include_directories absolute path validation Oct 22, 2023
@GertyP
Copy link
Contributor Author

GertyP commented Oct 22, 2023

I am concerned this is going to conflict

@dcbaker Are you referring to the suggestion to better detect truly problematic absolute paths - by tagging or tracking the path's consituent parts in a way that allows them to be inspected for things like particular dumb fixed string literals that would fail under source tree relocation - when pointing out your concern as to how that would fit in (or complicate) the subproject build+host machine issue you mention?
Or are you referring to the prospect of simply removing the existing abs path validation as being a concern for your issue?

@GertyP GertyP marked this pull request as ready for review October 22, 2023 17:08
@GertyP GertyP requested a review from jpakkane as a code owner October 22, 2023 17:08
The 'Tried to form an absolute path to a dir in the source tree' check
is a blunt instrument in that it's too simplistic to correctly identify
problematic, non-project-relative absolute path use without preventing
perfectly good and safe absolute include path use that makes for very
clean, maintainable/refactorable build files.

Guesing at potential problems and imposing a particular style on meson
users, particularly when those problems don't exist in practice, seems
like something best avoided.  The true arbiter of a problem path is
ultimately whether a path actually exists/works;  if they work, don't
add unnecessary obstacles for the user; if they don't work, the problem
and solution should be pretty clear to the user.

Discussion on this issue -
mesonbuild#12244
mesonbuild#12281
@dcbaker
Copy link
Member

dcbaker commented Nov 15, 2023

@GertyP, sorry for the very late reply.

Right now Meson will set up a perfect mirror between the source and build dir, so a layout looks like this:

subprojects/foo/include
builddir/subprojects/foo/include

Meson can ensure that this works correctly with absolutely paths.
With my series, a subproject can be built for the build machine as well as the host machine. so you end up with

subprojects/foo/include
builddir/subprojects/foo/include
builddir/subprojects.build/foo/include

I'm very concerned that allowing absolute paths rather than relative ones requires us to guarantee the layout of the project is a perfect mirror of the source dir, which is something we're currently not doing.

@GertyP
Copy link
Contributor Author

GertyP commented Nov 16, 2023

Thanks @dcbaker. No problem. I get the impression that there must be a large demand on just a very few people so I'm grateful to get a chance to broaden my understanding of use-cases that might complicate things.

Because I don't yet see your concern clearly, I think I need to try to break down a few things that you say in a bit more detail for me to fully understand the issue.

You say that "Meson can ensure that this works correctly with absolute paths": I find this a little confusing since meson would currently complain if, with your example, we had something like -

src_root_dir = meson.current_source_dir() / '../someproj'
inc_dirs = include_directories(src_root_dir / 'subprojects/foo/include')  # An explicit absolute path, but still safe and project source relative.

The current meson would not like this, even though it expresses exactly that we perfectly normally want to search for includes under the '[absolute_path_prefix_leading_to]/someproj/subprojects/foo/include' source dir.

I see use of absolute include dir paths to be very explicitly a statement of, "Don't try to be clever. Just use this one absolute path exactly as it is for compilation include directory options. Don't concern yourself with it; if things break from me carelessly relocating directories without any necessary fixing of hard-coded paths, that's on me; just throw it at the compiler and, sink or swim, leave it to the compiler to tell me whether we're good or not".

I do know that meson would be happy with relative paths like -

inc_dirs = include_directories('subprojects/foo/include')

causing it to implicitly add compile include path options for both your source dir 'subprojects/foo/include' AND your build-relative dir 'builddir/subprojects/foo/include'. If I needed to concern my builds with adding include search directories anywhere relative to the build directories (e.g. to find generated headers), which in my experience is certainly a minority case, then I'm perfectly happy to use relative include directories. Although I should add that in such cases, I'd know I'm referring to build-relative include directories so I'd want compilation to ONLY use the build include dir, not to sloppily double-up the compiler options with an unnecessary extra set of source relative include search dirs.

The implicit doubling up of include search paths (one for src-relative and one for build-relative) seems to me to be a different issue to what this PR is trying to improve, and your example of 2 sets of builddirs (host/target machine and build machine) does seem relevant to that.

This implicit doubling-up with include_directories is something that does not sit well with me and is something I've raised in a separate PR. Tldr: The user always know whether they intend source or build-relative (i.e. generated) include dirs. Explicit and strict is generally better than implicit and relaxed/sloppy. Why burden ninja and compilation with twice the number of -I args than necessary? Implicitly and forcibly doubling up src and build-relative include search dirs adds potential for new obscure problems like compilation picking up unintended build-relative headers over intended src-relative headers.

For that PR, your issue of what does meson do with host/target-machine build dir and build-machine build dir relative includes does seem relevant, although not being familiar with meson's current handling of this scenario, I'd struggle to properly mull over the issues. For example, does someone build (compile and link) both host and build machine binaries in one go in the same meson compile ... invocation and when compiling/linking, does meson internally abstract/hide whether 'the build dir' is actually pointing at 'builddir/subprojects' (if we're doing 'host machine' compiling) or whether 'the build dir' is actually 'builddir/subprojects.build' (if we're doing host machine compiling), so that meson's handling of builddir-relative include paths doesn't need to concern itself with host vs build machine build dirs; it just prefixes the opaque 'the build dir' to all build-relative includes and it'll substitute 'the build dir' with whichever's appropriate in the current compilation instance context (host machine vs build machine).

If, from all of the above, it's clear that I've not fully grasped your concerns, I hope it at least shows where the important gaps in my understanding might be. I also wonder if we could benefit from tweaking an existing 'test cases' project to actually illustrate whether this PR introduces a new issue that you're worried about.

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.

4 participants