Provide nix native way of creating a MariaDB database
Details
From any branch, on a darwin (MacOS) machine:
nix run 'github:commE2E/comm?ref=jonringer/mariadb#mariadb-up'
Diff Detail
- Repository
- rCOMM Comm
- Branch
- jonringer/mariadb
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
I'm not a good choice for a first-pass review except for these cases. Leaving some comments inline, but deferring most of the review to other people
nix/mariadb-up-mac.nix | ||
---|---|---|
41–135 ↗ | (On Diff #15343) | I assume that the ShellCheck CI doesn't have a way to parse this bash. Three questions:
|
66 ↗ | (On Diff #15343) | Why are we using single-bracket conditionals here? We spent a bunch of time trying to rid the codebase of these in ENG-157... please try to avoid regressing us. I think I recall mentioning this to you in the past, so please try to integrate it going forward, eg. please never put up a diff with single-bracket conditionals unless there's a strong reason |
nix/overlay.nix | ||
49 ↗ | (On Diff #15343) | Some quick questions about version locking, eg. what yarn.lock does:
|
Feedback for @jon – it often seems like you put stuff in test plan that people aren't able to reproduce. This causes churn for both you and reviewers, and I think we could save a lot of time if you were more diligent about making sure your test plans are reproducable for reviewers.
Feedback for @jon – it often seems like you put stuff in test plan that people aren't able to reproduce. This causes churn for both you and reviewers, and I think we could save a lot of time if you were more diligent about making sure your test plans are reproducable for reviewers.
Yea, was tired and wanted to go to bed, forgot to add my branch as a reference. My bad, and fixed.
nix/mariadb-up-mac.nix | ||
---|---|---|
41–135 ↗ | (On Diff #15343) |
Yes, it's bash. We can render the file, then check it
Not really, as nix needs to interpolate the paths of packages
Looking through some of the other "trivial builders", there's also a writeShellApplication which checks the content with shellcheck |
66 ↗ | (On Diff #15343) | because I was having a lot of issues why it was crashing on mariadb_count=$(ps aux | grep -v grep | grep -c mariadbd) thought it was somethign to do with the [[ ]] because I was desperate. Turns out grep normally fails if it finds 0 results. and forgot to undo this. |
nix/overlay.nix | ||
49 ↗ | (On Diff #15343) |
It locks every reference to mariadb. Since I will only be referencing mariadb, then it should be every dev's env.
No, generally patch versions have good fixes. We want those when we update nixpkgs. AFAIK, mariadb only does maintenance and security backports to minor versions.
Depends on the tool, I do something similar for protobuf. But most packages don't update in meaningful ways too often. Once we get some nix CI, we should be able to determine any regressions when updating nixpkgs, and react accordingly. |
Running into the following:
atul@atuls-MacBook-Pro comm % nix run github:commE2E/comm?ref=jonringer/mariadb#mariadb-up zsh: no matches found: github:commE2E/comm?ref=jonringer/mariadb#mariadb-up
For more information on writeShellApplication https://github.com/NixOS/nixpkgs/blob/68a2cf6583e73f355c70a27dd64913bb74038f03/pkgs/build-support/trivial-builders.nix#L254-L305
But it automatically sets set -euo pipefail that I've been doing in other scripts, and also runs the script through shellcheck when building it. So just doing nix build github:commE2E/comm?ref=jonringer/mariadb#mariadb-up (or any other nix command) also runs shellcheck
Also, for checking what nix is doing, you can also build it, and inspect the results
For example, seeing what interpreter it's using
$ nix build .#mariadb-up && head -n1 ./result/bin/mariadb-up #!/nix/store/qm2lv1gpbyn0rsfai40cbvj3h4gz69yc-bash-5.1-p16/bin/bash
nix/mariadb-up-mac.nix | ||
---|---|---|
41–135 ↗ | (On Diff #15343) |
I have no idea what this means. I fear we're slipping into a pattern again where I ask a question and get a really short sentence in response. Please make sure to respond to each request with enough detail that we don't end up churning during diff review. If you leave out the detail in your first response, you are only causing more work for everybody – your reviewers and you. To be absolutely clear, I am requesting that you should me the line of code that shows that this shell script is being executed as bash. Can you please link that line of code?
Can those things be provided as inputs into the bash script?
At runtime? That doesn't sound ideal... |
nix/overlay.nix | ||
49 ↗ | (On Diff #15343) |
I don't disagree, but doesn't this compromise Nix's promise of reproducable builds? It sounds like different devs will have different versions. I agree that patch versions have good fixes, but I would prefer a model where patch versions get pushed to each developer's environment when they are introduced. Instead, it appears that developers with the old version will not get the patched version... only people newly setting up the environment will get the patch versions. Or am I wrong? (Ideally I wouldn't have to ask for further clarification here... you can reduce churn by interpreting what I am trying to ask, and giving a full response that covers everything.) |
nix/mariadb-up-mac.nix | ||
---|---|---|
19 | Should the last line of this command end with a backslash? Not sure if this will execute. But it is the only command in the script, so I don't know. Going off of StackOverflow and StackExchange. | |
58 | Assuming this doesn't need to be double quoted since the entire script is in quotes already? Same assumption for lines 74, 100, 104, 107, 122, and 124? |
nix/mariadb-up-mac.nix | ||
---|---|---|
19 | Yep, I had stdout and stderr on separate lines before, thanks | |
58 | Correct, nix store paths are computed and can't have spaces in them $ nix eval nixpkgs#hello.outPath "/nix/store/j943c0z8m4qhx1vr3zzh91fjz1bnhg1h-hello-2.12.1" | |
41–135 ↗ | (On Diff #15343) |
I expanded on this in a separate comment: https://phab.comm.dev/D4750#136994 But it probably should be part of the original response It inherits the shell from the stdenv environment, which is going to contain bash v5.1
$ nix build github:commE2E/comm?ref=jonringer/mariadb#mariadb-up && head -n1 ./result/bin/mariadb-up #!/nix/store/qm2lv1gpbyn0rsfai40cbvj3h4gz69yc-bash-5.1-p16/bin/bash
It's a build time. In another comment, I linked to the nixpkgs definition of writeShellApplication which shows that it create a package where the contents of script are ran against shellcheck as part of the checkPhase (which is usually used for test suites). In other words, writeShellApplication takes the scripts, runs it through a few tests (one of which is shellcheck), then re-exposes if it passed. If it failed any test then you would see any nix command trying to make use of it fail.
Possible, but that's unusual. Are we trying to solve shellcheck or trying to solve script re-use? If we're trying to solve shellcheck, then writeShellApplication already does this for us, so that's not much of a win. If we wanted to make the scripts more portable, then I think refactoring the script to just use PATH, and expose a nix wrapper script which sets the correct PATH then calls the portable script would make sense. However, I don't think we want to go down the path of supporting many different development environments. |
nix/overlay.nix | ||
49 ↗ | (On Diff #15343) |
Not really, reproducible just means starting from a known location A can you always get to same exact location B? If the anwser is yes, then it's reproducible. For nix, only updating the nix lock file or the overlay should really be affecting which packages get introduced into the environment. If programmer A has the same flake.lock and nix/overlay.nix as programmer B, then both programmers will have the exact same (usually byte-for-byte same) packages. I think you are more concerned about mariadb_108 moving between developers. But mariadb_108 is determined by nixpkgs, which is frozen to a particular commit in the flake.lock file. One of my youtube videos on flakes might help with understanding how this works a bit (although I do make the assumption that you've dabbled with nix) https://youtu.be/90P-Ml1318U?t=90
If they have the same flake.lock and nix/overlay.nix then no, the packages should be exactly the same. But if we do need to update the package, then we can make the change in either the flake.lock (if nixpkgs has what we want) or the overlay(if nixpkgs doesn't have what we want); then ask "please rebase on the latest master and run nix develop again".
The patch version will likely be decided by the flake.lock which contains a reference to a specific checkout of nixpkgs. I'm most refering to when we do update the lock file, it will likely bump the patch version.
For the purpose of the mariadb package which nix will reference, that is determined by the nixpkgs reference in the flake.lock and anything we might do in the overlay. As we only update the flake.lock every few months, it's more likely that individual users will have the exact same version. |
Remove extraneous backslash
Previously had a line for both stdout and stderr, and forgot
to remove the newline continuation after combining the file handles
Sorry to request changes again, but still unable to get this to work in my environment:
atul@atuls-MacBook-Pro comm % nix run github:commE2E/comm?ref=jonringer/mariadb#mariadb-up zsh: no matches found: github:commE2E/comm?ref=jonringer/mariadb#mariadb-up
Any ideas how I can resolve this?
zsh: no matches found: github:commE2E/comm?ref=jonringer/mariadb#mariadb-up
I guess this is related to zsh https://github.com/NixOS/nix/issues/4686
Either, do nix develop which will drop you into a bash shell, or you can single quote the "flake url". I'll update the comment
Some small fixes here and there, then should be good to go!
nix/mariadb-up-mac.nix | ||
---|---|---|
58 ↗ | (On Diff #15382) |
In response to this reply to my inline comment, won't ShellCheck still complain about this not being double quoted though? Even though we know Nix store paths can't have spaces in them, we should still double quote for consistency/good style. Other instances of double quoting ShellCheck complains about on lines 74, 100, 104, 107, 122, and 124. |
65–66 ↗ | (On Diff #15382) | So just to be clear, the wc -l will still capture the count of the number of pgrep results, but with whitespace? And that's why we need the xargs echo? Not sure if it's better, but we could also use something like awk, like piping it to awk {print $1}? |
69 ↗ | (On Diff #15382) |
This should be double brackets, right? |
83 ↗ | (On Diff #15382) | Are double quotes necessary here? We use them every time we reference running_pid later anyways. |
87 ↗ | (On Diff #15382) | You say "stop or kill" here but only "stop" on line 93, should the verbs for both match? Same thing goes for saying "attempt 'mariadb-up'" here but "attempt 'mariadb-up' again" on line 93. |
101 ↗ | (On Diff #15382) | I don't think there's a difference between COUNT(1) and COUNT(*) (at least semantically), but there are definitely more instances of COUNT(*) in the codebase than COUNT(1). Is there a reason we can't use COUNT(*)? Maybe performance? I prefer COUNT(*) since it makes more sense to me as a reader, but I could be wrong since my SQL experience is one quarter of college :P |
108 ↗ | (On Diff #15382) | nit: include spaces between flags and their arguments. You will also probably have to add a space to the following two lines so they're aligned. |
nix/mariadb-up-mac.nix | ||
---|---|---|
65–66 ↗ | (On Diff #15382) | wc -l counts the number of lines returned.
I'm fine with this solution as well, but I find that less people know about awk than wc + xargs |
69 ↗ | (On Diff #15382) | yes |
83 ↗ | (On Diff #15382) | no, but shellcheck complains |
87 ↗ | (On Diff #15382) | I'll just take 93 |
101 ↗ | (On Diff #15382) | Depends on the query optimizer. For MariaDB I would assume that it would know that the tuple info gets discarded, but COUNT(1) is a bit more explicit that the information of the user table is irrelevant. |
Ok, bash-wise, looks good!
nix/mariadb-up-mac.nix | ||
---|---|---|
65–66 ↗ | (On Diff #15382) |
Haha, yeah. I guess that's true, this is awk |