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

Die, layout=flat, die #9243

Merged
merged 2 commits into from
Sep 12, 2021
Merged

Conversation

eli-schwartz
Copy link
Member

@eli-schwartz eli-schwartz commented Sep 12, 2021

The option for layout=flat is horribly broken beyond repair in various circumstances which no one really keeps track of, no one seems to actually use it in practice for any good reason, CI doesn't test it, no one is committed to maintaining it, etc. etc. etc.

How is it broken? Well:

  • projects doing trivially reasonable things, such as generating "foo/util.py" and "bar/util.py", create clashing output names. This will never, ever, ever, ever work with layout=flat. This is by design, because they are fundamentally incompatible goals.
  • Generally, meson assumes all over the place that files go in their subdir output directories, and since it is never tested in CI, these assumptions go unnoticed...
  • Sometimes meson itself generates files in subdir-based layouts, for example my recent changes to the i18n module produce dozens of files all named "project.mo" but in directories distinguished by unique locales. This is because locale files are all named the same thing, and gettext chooses the right file by searching a locale directory... there is nothing we can do here! There are probably also other generated cases, but who even cares to try to figure out what they are???

I propose that layout=flat is a failed experiment. It only ever existed in the first place, because... Windows. Windows is technically broken. It relies on PATH to find dlls which executables depend on, so programs don't run while uninstalled if subdirs are in play. Enter layout=flat just stick everything all in one directory, surely nothing can possibly go wrong... except it does.

There is a much better way to handle this, and recent versions of meson include it: meson devenv will let you enter an environment where your PATH is set up properly and you can run uninstalled programs. Let's deprecate and remove layout=flat in favor of devenv. Everyone will thank us.

Closes #996
Closes #1521
Closes #1908
Closes #7133
Closes #7135
Closes #7480
Closes #8378

(and probably more, who knows)

Until we invoke interpreter.Interpreter(b, ...) the coredata options
still have their default values and thus cannot be used sensibly.
Currently the warning never shows (other than, unsurprising in
retrospect, during --internal regenerate).
@eli-schwartz eli-schwartz marked this pull request as draft September 12, 2021 06:45
@codecov
Copy link

codecov bot commented Sep 12, 2021

Codecov Report

Merging #9243 (3cddb0e) into master (f291b63) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9243      +/-   ##
==========================================
- Coverage   66.85%   66.85%   -0.01%     
==========================================
  Files         386      386              
  Lines       84990    84994       +4     
  Branches    17535    17537       +2     
==========================================
+ Hits        56824    56826       +2     
- Misses      23374    23376       +2     
  Partials     4792     4792              
Impacted Files Coverage Δ
mesonbuild/msetup.py 89.36% <50.00%> (+0.11%) ⬆️
mesonbuild/scripts/vcstagger.py 87.50% <0.00%> (-4.17%) ⬇️
mesonbuild/mesonlib/universal.py 81.81% <0.00%> (-0.09%) ⬇️
msetup.py 89.36% <0.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f291b63...3cddb0e. Read the comment docs.

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.

This looks good to me. I agree, unless someone wants to step up and get this working and in CI, removing it seems like the right decision to me.

It's broken in various circumstances, no one seems to actually use it,
CI doesn't test it, no one is committed to maintaining it, etc. etc.
etc.

Also, projects doing trivially reasonable things, such as generating
"foo/util.py" and "bar/util.py", create clashing output names. This will
never, ever, ever, ever work with layout=flat.

Closes mesonbuild#996
Closes mesonbuild#1521
Closes mesonbuild#1908
Closes mesonbuild#7133
Closes mesonbuild#7135
Closes mesonbuild#7480
Closes mesonbuild#8378
@eli-schwartz eli-schwartz marked this pull request as ready for review September 12, 2021 14:52
@eli-schwartz eli-schwartz merged commit 3cddb0e into mesonbuild:master Sep 12, 2021
@eli-schwartz eli-schwartz deleted the die-layout-flat-die branch September 12, 2021 16:12
@xclaesse
Copy link
Member

@eli-schwartz Seems worth adding a release snippet.

marzer pushed a commit to marzer/tomlplusplus that referenced this pull request Sep 22, 2021
Use the / operator instead of join_paths

Use the / operator instead of using "/" in string paths

Use the includedir opt instead of hardcoding "include" in install_subdir

Remove discouraged layout=flat option (mesonbuild/meson#9243)

Remove unneeded Wextra, Wpedantic flags, already added by warning_level

Remove manual -Oz flag when using Clang (mesonbuild/meson#9286)

Make use of : in kwargs consistent
@tabedzki
Copy link

tabedzki commented Aug 16, 2022

I only recently started using meson and thought this would be nice to have. However, since this is a pain to maintain and has been deprecated, let's remove it from https://mesonbuild.com/Builtin-options.html so new users don't accidentally stumble upon it.

Alternatively, is there a way to specify that a target should be built in the toplevel domain to mimic layout=flat putting the binary in the top directory?

@eli-schwartz
Copy link
Member Author

One thing some people do is use subdir() to collect files into a variable, and then define the actual target in the toplevel meson.build using a much simpler one-line declaration that doesn't use bulky filelists.

@tabedzki
Copy link

Pardon my inexperience, but how do you do that? I have a gpu_files=['file1', 'file2'] within the src/meson.build. Do I just need to append a 'src/` in front of all the files for that?

@eli-schwartz
Copy link
Member Author

eli-schwartz commented Aug 16, 2022

gpu_files = files('file1', 'file2')

You can now use the gpu_files variable from anywhere, any subdir at all, and it is guaranteed to always correctly refer to the right files.

Unlike strings, a files object has an associated source directory that it is considered relative to. Using files() is not necessary when passing literal arguments into a function that accepts "strings referring to files" or "files", but it's very handy when passing variables around.

@tabedzki
Copy link

tabedzki commented Aug 16, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants