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

Misc Meson progress #11302

Merged
merged 1 commit into from
Aug 27, 2024
Merged

Misc Meson progress #11302

merged 1 commit into from
Aug 27, 2024

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Aug 14, 2024

Motivation

Meson-ify a few things, scripts, completions, etc. Should make our Meson build complete except for docs.

Context

depends on #11301

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority labels Aug 14, 2024
@Ericson2314 Ericson2314 force-pushed the meson-misc-2 branch 4 times, most recently from 736c7f4 to 6ef1bca Compare August 14, 2024 22:05
Copy link

dpulls bot commented Aug 15, 2024

🎉 All dependencies have been resolved !

@Ericson2314
Copy link
Member Author

@eli-schwartz Do you know what I might do about this macos failure? Seems like the issue is that the is_dir check in https://github.com/mesonbuild/meson/blob/1e2e30bbcfea876e05ce5b863579dc7eb71b5fb3/mesonbuild/interpreter/interpreter.py#L704-L715 is failing because of sandboxing.

Copy link
Contributor

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

python pathlib has an alarming tendency to raise nonsense exceptions when it should not. Maybe this is another one...

src/nix/meson.options Outdated Show resolved Hide resolved
@@ -421,4 +421,9 @@ install_headers(headers, subdir : 'nix', preserve_path : true)

libraries_private = []

extra_pkg_config_variables = {
'localstatedir' : get_option('state-dir'),
Copy link
Contributor

Choose a reason for hiding this comment

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

And here you call it localstatedir! :D

Which makes me wonder why you don't use meson's own --localstatedir option (https://www.gnu.org/prep/standards/standards.html#Directory-Variables, even) and just set that value to /nix/var, perhaps in default_options.

project('nix-store', 'cpp',
version : files('.version'),
default_options : [
'cpp_std=c++2a',
# TODO(Qyriad): increase the warning level
'warning_level=1',
'debug=true',
'optimization=2',
'errorlogs=true', # Please print logs for tests that fail
],
meson_version : '>= 1.1',
license : 'LGPL-2.1-or-later',
)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh hmm, I just took this from Lix's Meson, but maybe the Lix devs didn't realize one could override a built-in option default value. CC @Qyriad

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps:

diff --git a/src/libstore/meson.build b/src/libstore/meson.build
index d0cd7d3cb..b829de3f7 100644
--- a/src/libstore/meson.build
+++ b/src/libstore/meson.build
@@ -421,9 +421,9 @@ install_headers(headers, subdir : 'nix', preserve_path : true)
 
 libraries_private = []
 
-extra_pkg_config_variables = {
-  'localstatedir' : get_option('state-dir'),
-  'storedir' : get_option('store-dir'),
-}
+extra_pkg_config_variables = [
+  f'localstatedir=' + get_option('state-dir'),
+  f'storedir=' + get_option('store-dir'),
+]
 
 subdir('build-utils-meson/export')

We do the dict to string conversion ourselves and it doesn't look like a Path, thus bypassing that check.

Copy link
Member Author

Choose a reason for hiding this comment

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

Trying @eli-schwartz's localstatedir=/nix/var suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

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

Still a problem. I minimized it and opened mesonbuild/meson#13584 Meson options are a red herring -- it's declared dependency variables instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@eli-schwartz Well, I am open to alternatives! Here's basically what I am thinking:

  1. The paths are a property of the libstore library, and configurable, and needs to be conveyed downstream.

  2. We can't just put it in a header, because we need it at configure time downstream, patting localstatedir and storedir in the pkg-config file seems more appropriate.

  3. When we are doing the combined subprojects build, we don't use a pkg-config dep, and instead use an internal dep. So we get the variables from the internal dep instead.

Where does it go off the rails to you, conceptually? Each step seems fine to me, even if the end result is a bit funky.

Copy link
Contributor

Choose a reason for hiding this comment

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

When you're doing the combined subprojects build, do different subprojects get built with different prefix/libdir/localstatedir?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair question: Nope. Each project should either use the same localstatedir, or doesn't care about the variable at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

So perhaps for the purpose of this PR we can dodge the question of what declare_dependency is doing, and simply have projects that want this value look it up via get_option() instead of subp_dep.get_variable()...

Copy link
Member Author

Choose a reason for hiding this comment

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

I did something along that lines. I still need the pkg-config variable for storedir, but I now don't set the localstatedir one on macOS, and I have an assert that it lines up with get_option('localstatedir').

@Ericson2314
Copy link
Member Author

python pathlib has an alarming tendency to raise nonsense exceptions when it should not. Maybe this is another one...

I think the (at least longer term) solution might be a Meson-side try-catch, as permission error is indeed different from knowing for sure whether something is a directory or not, but I'm not quite sure what the meson logic is doing there so I'm not sure I'd be patching it right.

Meson-ify a few things, scripts, completions, etc. Should make our Meson
build complete except for docs.

Co-Authored-By: Qyriad <qyriad@qyriad.me>
Co-Authored-By: eldritch horrors <pennae@lix.systems>
@roberth roberth merged commit 076b6f7 into NixOS:master Aug 27, 2024
11 checks passed
@Ericson2314 Ericson2314 deleted the meson-misc-2 branch August 27, 2024 18:56
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-08-26-nix-team-meeting-minutes-172/51300/1

configuration : {
'storedir' : store_dir,
'localstatedir' : localstatedir,
'bindir' : get_option('datadir'),
Copy link
Member

Choose a reason for hiding this comment

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

This appears to produce a busted systemd unit.

$ cat result/lib/systemd/system/nix-daemon.service
[Unit]
Description=Nix Daemon
Documentation=man:nix-daemon https://nixos.org/manual
RequiresMountsFor=/nix/store
RequiresMountsFor=/nix/var
RequiresMountsFor=/nix/var/nix/db
ConditionPathIsReadWrite=/nix/var/nix/daemon-socket

[Service]
ExecStart=@share/nix-daemon nix-daemon --daemon
KillMode=process
LimitNOFILE=1048576
TasksMax=1048576

[Install]
WantedBy=multi-user.target

Notice how ExecStart is @share/nix-daemon, which is unlikely to exist on any machine.

Fixed in #11413.

YorikSar added a commit to tweag/nix that referenced this pull request Sep 17, 2024
Workaround at src/libstore/meson.build#L429-L434 by @Ericson2314 from
NixOS#11302 erroneously used `macos` instead
of `darwin` to distinguish macOS, while meson docs list only `darwin`:
https://mesonbuild.com/Reference-tables.html#operating-system-names.

Original thread: NixOS#2503 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants