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

Deprecate meson.build_root() and meson.source_root() #7772

Merged
merged 2 commits into from
Sep 29, 2020

Conversation

xclaesse
Copy link
Member

Those function are common source of issue when used in a subproject because they
point to the parent project root which is rarely what is expected and is a
violation of subproject isolation.

Those function are common source of issue when used in a subproject because they
point to the parent project root which is rarely what is expected and is a
violation of subproject isolation.
@xclaesse
Copy link
Member Author

On IRC it was suggested to add meson.project_source/build_root() to replace them. I personally think that it's easy enough to declare a variable source_root = meson.current_source_dir() in the root meson.build for projects that does lots of paths relative to the root.

What does others think?

@xclaesse
Copy link
Member Author

@jpakkane what do you think?

@xclaesse
Copy link
Member Author

One problem in adding new API is it force projects to depend on latest meson to get ride of a deprecation warning, while current_source_dir() is available for a long time already (it has no "since" annotation).

@xclaesse xclaesse added this to the 0.56.0 milestone Sep 23, 2020
@kbingham
Copy link
Contributor

If the feature addition goes in at the same time, I don't think it's a problem as the feature is available at the point of deprecation.... ?

To me it seems like a candidate to have these project specific variables defined, But that's purely as a convenience I guess.
I know I'll end up having to define these variables in the root meson.build file, as we use those now deprecated values in our subdirs ...

So if 'I' need it, I would assume it is needed elsewhere ... ?

@kbingham
Copy link
Contributor

I guess an alternative is to make meson.build_root() and meson.source_root() relative to the current project - but I guess that could cause further implications as it becomes a change of definition.

I don't know what use cases a subproject would want to access a parent project - as I assume it shouldn't know about the parent project at all... so that makes me believe build_root() and source_root() should always be project relative? (but I there's a lot I don't know on this)

@xclaesse
Copy link
Member Author

I'm also tempted to change behavior of meson.build_root(), but from experience people does all kind of crazy hacks with their build system, there are chances it backfires at us, IMHO. Better not taking risks when alternative is already available.

@eli-schwartz
Copy link
Member

I think most people thought it would be project-specific, but who knows what a small handful of people are doing for crazy reasons. :p

My thought in suggesting a new function is that it was considered desirable enough to implement the global version in the first place, i.e. people want it and it's a useful enough convenience to merit its own sugar. So by extension, it's useful enough to reimplement it the way it was always meant to behave, rather than force people to define the variable in a different-level meson.build file.

@lygstate
Copy link
Contributor

lygstate commented Sep 25, 2020

I think most people thought it would be project-specific, but who knows what a small handful of people are doing for crazy reasons. :p

My thought in suggesting a new function is that it was considered desirable enough to implement the global version in the first place, i.e. people want it and it's a useful enough convenience to merit its own sugar. So by extension, it's useful enough to reimplement it the way it was always meant to behave, rather than force people to define the variable in a different-level meson.build file.

How about rename to meson.global_build_root() and meson.global_source_root()

Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

I'm good with deprecating these, they are pretty much useless. Im not sure about the replacement, I've never had a use case for it but if we're going to add it that should happen in the 0.56

@jpakkane
Copy link
Member

I'm also in favor of deprecating these. We can add global_source_root/project_source_root if people file feature requests to add them.

@xclaesse
Copy link
Member Author

My thought in suggesting a new function is that it was considered desirable enough to implement the global version in the first place, i.e. people want it and it's a useful enough convenience to merit its own sugar. So by extension, it's useful enough to reimplement it the way it was always meant to behave, rather than force people to define the variable in a different-level meson.build file.

Fair enough, added project_build/source_root() now.

How about rename to meson.global_build_root() and meson.global_source_root()

That would not be backward compatible. We don't want to break existing projects that are not always meant to be used as a subproject.

I'm also in favor of deprecating these. We can add global_source_root/project_source_root if people file feature requests to add them.

I think getting the global root is wrong in all cases, it violates subproject isolation and if you're doing that on purpose you probably deserve getting a deprecation warning. That being said, if it turns out we get bug reports with valid use-cases that I didn't though about then we'll add them, yes.

@emersion
Copy link
Contributor

emersion commented Feb 8, 2021

The meson.build_root function is used in one of my projects to strip a prefix from __FILE__. Using current_build_dir() or project_build_root() doesn't work.

https://github.com/swaywm/wlroots/blob/d595a4ebe3d432cdd3f551d6d067b341d0afdc19/meson.build#L45

@xclaesse
Copy link
Member Author

xclaesse commented Feb 9, 2021

Hmmm, that sounds like a legitimate use-case actually, didn't though about that. @jpakkane @dcbaker what do you think? Should we un-deprecate? Or add a new method with a more descriptive name like top_source_root()?

@jpakkane
Copy link
Member

jpakkane commented Feb 9, 2021

From what I can tell that code is only using source and build dirs to calculate the relative path from build to source. What if we exposed that directly so you don't have to roll it by hand?

@emersion
Copy link
Contributor

Or add a new method with a more descriptive name like top_source_root()?

Sounds good to me.

What if we exposed that directly so you don't have to roll it by hand?

Sounds good to me as well, if there aren't more use-cases that really need build_root.

@tp-m
Copy link
Member

tp-m commented Feb 16, 2021

I have another use case where I actually need build_root, namely to construct the path to $build_root/meson-info/intro-buildoptions.json (in a subproject), so I think a global_build_root() would be nice at least.

emersion added a commit to emersion/meson that referenced this pull request Apr 8, 2021
There are valid use-cases for these functions.

References: mesonbuild#7772
@xclaesse xclaesse deleted the deprecate-source-root branch October 20, 2021 14:44
@nomis
Copy link
Contributor

nomis commented Apr 23, 2022

https://mesonbuild.com/Localisation.html#mesonbuild still refers to meson.source_root() in its example, although it doesn't state why this is needed

@eli-schwartz
Copy link
Member

eli-schwartz commented Apr 24, 2022

It is absolutely, positively, not needed. Because Meson internally passes ['--directory', os.environ['MESON_SOURCE_ROOT']] inside the <foo>-pot target.

I think this documentation is entirely wrong. :) But it might be trying to demonstrate advanced usage of the i18n.gettext() function, though why it needs to do that in a "basics" tutorial I don't know...

@nomis
Copy link
Contributor

nomis commented Apr 24, 2022

It is absolutely, positively, not needed. Because Meson internally passes ['--directory', os.environ['MESON_SOURCE_ROOT']] inside the <foo>-pot target.

Could you provide a reference to where Meson does this because I can't find it?

mesonbuild/scripts/gettext.py:run_potgen

Meson has always provided this (as -D) so supplying the same value twice does nothing. gettext adds unique values to a list.

PR: #10307

@nomis
Copy link
Contributor

nomis commented Apr 24, 2022

https://mesonbuild.com/Dlang-module.html refers to meson.source_root() too.

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.

None yet

9 participants