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

Build manual with Meson #11224

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from
Draft

Build manual with Meson #11224

wants to merge 10 commits into from

Conversation

Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jul 31, 2024

Motivation

Meson for:

  • doc/manual
  • scripts
  • misc

Context

#2503

Depends on #11073
Depends on #11302

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 documentation new-cli Relating to the "nix" command contributor-experience Developer experience for Nix contributors with-tests Issues related to testing. PRs with tests have some priority repl The Read Eval Print Loop, "nix repl" command and debugger labels Jul 31, 2024
@L-as
Copy link
Member

L-as commented Jul 31, 2024

What does this change compared to the other PRs? Wish GitHub supported dependencies in PRs natively.

@Ericson2314
Copy link
Member Author

@L-as Edited the description to be precise about it.

Copy link
Member

Choose a reason for hiding this comment

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

Oof, this sounds like a bug? Do meson or ninja have an issue about this we can follow?

assert '\\' not in path
assert ' ' not in path
assert '#' not in path
print("ignored:", path)
Copy link
Member

Choose a reason for hiding this comment

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

What does ignored mean?

Comment on lines 4 to 7
This script is a helper for this project's Meson buildsystem, to replace its
usage of `nix eval --write-to`. Writing a JSON object as a nested directory
tree is more generic, easier to maintain, and far, far less cursed. Nix
has 'good' support for JSON output. Let's just use it.
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a motivation for a change, not actual documentation of what it does. Also not sure what curses have to do with this.

What is the file format and the result? Could you give an example so contributors can easily form an intuition for this program?

Copy link
Member

Choose a reason for hiding this comment

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

Thought: is this a File System Object?

];

# Hack for sake of the dev shell
passthru.baseNativeBuildInputs = [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
passthru.baseNativeBuildInputs = [
passthru.externalNativeBuildInputs = [

This seems to be the defining feature, and what we want to have in the dev shell.
I can imagine an exception where we have an infrequently changed custom development tool that's actually local, but want to treat it as external for practicality, but we can revisit the name when we get there, and that's actually about the dev shell and not about this attribute anyway, so let's go with external for the package attribute?

jq
];

nativeBuildInputs = finalAttrs.passthru.baseNativeBuildInputs ++ [
Copy link
Member

Choose a reason for hiding this comment

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

thought: This could be factored out into mkMesonDerivation, or one of its layers, or maybe it should be called mkNixMesonDerivation or something because it's getting a bit specific.

if (runNixPtr)
(*runNixPtr)(program, args, input);
else
throw Error("Cannot run '%s', no method of calling the Nix CLI provided", program);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw Error("Cannot run '%s', no method of calling the Nix CLI provided", program);
throw Error("Cannot run '%s' because no method of calling the Nix CLI was provided. This is a configuration problem pertaining to how this program was built. See Nix 2.25 release notes.", program);
  • pet peeve: unidiomatic use of comma
  • let's give some context

Ideally we could be more specific and give a URL.

Copy link
Member

Choose a reason for hiding this comment

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

you could also use a semicolon to keep the brevity

Comment on lines 252 to 258
subdir('scripts')
subdir('misc')
Copy link
Member

Choose a reason for hiding this comment

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

I would like these dependencies to be inverted if possible. I don't think the CLI build should be the final do-it-all derivation, because it means that changing these configs requires a CLI rebuild, making those files unnecessarily slow to iterate on.

if (auto envOpt = getEnvNonEmpty("NIX_BIN_DIR"))
return fs::path{*envOpt} / std::string{getBinaryName()};

// Use some-times avaiable OS tricks to get to the path of this Nix, and try that
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Use some-times avaiable OS tricks to get to the path of this Nix, and try that
// Try OS tricks if available

* Instead, we'll query the OS for the path to the current executable,
* using `getSelfExe()`.
*
* As a last resort, we resort to `PATH`. Hopefully we find a `nix`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* As a last resort, we resort to `PATH`. Hopefully we find a `nix`
* As a last resort, we rely on `PATH`. Hopefully we find a `nix`

'PS4': '+(${BASH_SOURCE[0]-$0}:$LINENO) ',
},
# some tests take 15+ seconds even on an otherwise idle machine, on a loaded machine
# this can easily drive them to failure. give them more time, 5min rather than 30sec
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# this can easily drive them to failure. give them more time, 5min rather than 30sec
# this can easily drive them to failure. give them more time than the default 30sec

Bitrot-proofing the comment.

Copy link

dpulls bot commented Aug 14, 2024

🎉 All dependencies have been resolved !

@Ericson2314 Ericson2314 force-pushed the meson-misc branch 2 times, most recently from 7688ec6 to 535f68a Compare August 19, 2024 15:20
@Ericson2314 Ericson2314 changed the title Meson misc Build manual with Meson Aug 19, 2024
Copy link

dpulls bot commented Aug 27, 2024

🎉 All dependencies have been resolved !

Ericson2314 and others added 2 commits September 16, 2024 10:05
Co-Authored-By: Qyriad <[email protected]>
Co-Authored-By: eldritch horrors <[email protected]>
@Ericson2314
Copy link
Member Author

Added the commit that @haenoe contributed, thanks!

@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-09-18-nix-team-meeting-minutes-179/52361/1

@Ericson2314
Copy link
Member Author

Oh lovely, I don't think I got that eval error locally with Nix 2.18.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor-experience Developer experience for Nix contributors documentation new-cli Relating to the "nix" command repl The Read Eval Print Loop, "nix repl" command and debugger 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.

5 participants