Skip to content

fio-plot: 1.1.16#435004

Open
johnrichardrinehart wants to merge 3 commits intoNixOS:masterfrom
johnrichardrinehart:fio_plot
Open

fio-plot: 1.1.16#435004
johnrichardrinehart wants to merge 3 commits intoNixOS:masterfrom
johnrichardrinehart:fio_plot

Conversation

@johnrichardrinehart
Copy link
Contributor

@johnrichardrinehart johnrichardrinehart commented Aug 19, 2025

Sorry ahead-of-time if this PR is absolute crap. I have little to no experience with python packaging and I have attention issues, so there may be a lot of small things to clean up, here. Thanks for your review.

The goal of this PR is to add fio-plot (a package which provides two binaries, fio-plot and bench_fio), to legacyPackages. I've only tested that the binaries build and seem to run on x86_64-linux:

$ ./result/bin/bench_fio                                                                                                                
 
usage: bench_fio [-h] -d TARGET [TARGET ...] -t {device,file,directory,rbd} [-P CEPH_POOL] [-s SIZE] -o OUTPUT [-b BLOCK_SIZE [BLOCK_SIZE ...]]
                 [--iodepth IODEPTH [IODEPTH ...]] [--numjobs NUMJOBS [NUMJOBS ...]] [--runtime RUNTIME] [-p] [--precondition-repeat]
                 [--precondition-template PRECONDITION_TEMPLATE] [-m MODE [MODE ...]] [--rwmixread RWMIXREAD [RWMIXREAD ...]] [-e ENGINE] [--direct DIRECT] [--loops LOOPS]
                 [--time-based] [--entire-device] [--ss SS] [--ss-dur SS_DUR] [--ss-ramp SS_RAMP] [--extra-opts EXTRA_OPTS [EXTRA_OPTS ...]] [--invalidate INVALIDATE]
                 [--quiet] [--loginterval LOGINTERVAL] [--dry-run] [--destructive] [--remote REMOTE] [--remote-checks] [--remote-timeout REMOTE_TIMEOUT] [--create]
                 [--parallel]
bench_fio: error: the following arguments are required: -d/--target, -t/--type, -o/--output
$ ./result/bin/fio-plot 
usage: fio-plot [-h] -i INPUT_DIRECTORY [INPUT_DIRECTORY ...] [-o OUTPUT_FILENAME] -T TITLE [-s SOURCE] (-L | -l | -N | -H | -g | -C) [--disable-grid] [--enable-markers]
                [--subtitle SUBTITLE] [-d IODEPTH [IODEPTH ...]] [-n NUMJOBS [NUMJOBS ...]] [-M [MAXDEPTH]] [-J [MAXJOBS]] [-D [DPI]] [-p [PERCENTILE]]
                -r {read,write,randread,randwrite,randrw,trim,rw,readwrite,randtrim,trimwrite} [-m MAX_Z] [-e MOVING_AVERAGE] [--min-iops MIN_IOPS] [--min-lat MIN_LAT]
                [-t {bw,iops,lat,slat,clat} [{bw,iops,lat,slat,clat} ...]] [-f {read,write} [{read,write} ...]] [--truncate-xaxis TRUNCATE_XAXIS]
                [--xlabel-depth XLABEL_DEPTH] [--xlabel-parent XLABEL_PARENT] [--xlabel-segment-size XLABEL_SEGMENT_SIZE] [--xlabel-single-column] [-w LINE_WIDTH]
                [--group-bars] [--show-cpu] [--show-data] [--show-ss] [--table-lines] [--max-lat MAX_LAT] [--max-clat MAX_CLAT] [--max-slat MAX_SLAT] [--max-iops MAX_IOPS]
                [--max-bw MAX_BW] [--draw-total] [--colors COLORS [COLORS ...]] [--disable-fio-version] [--title-fontsize TITLE_FONTSIZE]
                [--subtitle-fontsize SUBTITLE_FONTSIZE] [--source-fontsize SOURCE_FONTSIZE] [--credit-fontsize CREDIT_FONTSIZE] [--table-fontsize TABLE_FONTSIZE]
                [--include-hosts INCLUDE_HOSTS [INCLUDE_HOSTS ...] | --exclude-hosts EXCLUDE_HOSTS [EXCLUDE_HOSTS ...]]
fio-plot: error: the following arguments are required: -i/--input-directory, -T/--title, -r/--rw

Now, in order to provide the application package, I also needed to add the corresponding python module package, fio_plot. But, fio_plot depends on https://github.com/Technologicat/pyan/, which is not in tree, yet.

Also, I was running into weird issues around bin files already existing when the fio_plot repository only provided its setup.py. The world seems to be moving toward pyproject.toml so you might notice that I've also bundled in a patch to the source repository which provides a pyproject.toml. This file unblocks the build. I'm going to make a corresponding PR to https://github.com/louwrentius/fio-plot/, but I'm not sure if this project is still maintained. I also want to be able to point to this PR in that PR and say "hey, I needed to hack something up in nixpkgs because your project packaging is stale". I'm happy to remove this patch later if the source repository includes these changes.

So, rather then break this into 3 separate/stacked PRs, I decided to provide a single PR. I hope that's okay.

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

Copy link
Contributor

@RossSmyth RossSmyth left a comment

Choose a reason for hiding this comment

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

Mostly good, a few little changes:

  1. Some sanity checks
  2. Remove with lib;
  3. Use python3Packages rather than python3.pkgs because the latter doesn't splice correctly
  4. Is there an upstream PR for the pyproject.toml? I would recommend putting your patch there if not, then fetching it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
python,

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
python.pkgs.buildPythonPackage rec {
buildPythonPackage rec {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
build-system = [ python.pkgs.setuptools ];
build-system = [ setuptools ];

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dependencies = with python.pkgs; [
dependencies = [

Comment on lines 34 to 76
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
meta = with lib; {
description = "Create charts from FIO storage benchmark tool output";
homepage = "https://github.com/louwrentius/fio-plot";
license = licenses.bsd3;
maintainers = with maintainers; [ johnrichardrinehart ];
};
meta = {
description = "Create charts from FIO storage benchmark tool output";
homepage = "https://github.com/louwrentius/fio-plot";
license = lib.licenses.bsd3;
maintainers = with lib.maintainers; [ johnrichardrinehart ];
};

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fio_plot = python3.pkgs.fio_plot;
fio_plot = python3Packages.fio_plot;

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
python3.pkgs.buildPythonApplication rec {
python3Packages.buildPythonApplication rec {

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
build-system = [ python3.pkgs.setuptools ];
build-system = [ python3Packages.setuptools ];

Comment on lines 40 to 41
Copy link
Contributor

Choose a reason for hiding this comment

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

Or whever it is called

Suggested change
maintainers = with maintainers; [ johnrichardrinehart ];
};
maintainers = with maintainers; [ johnrichardrinehart ];
mainProgram = "fio-plot";
};

Copy link
Contributor

Choose a reason for hiding this comment

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

What's this wip file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, man, haha. That shouldn't be in the history. It's the exact same patch as in the library package (python-modules/fio_plot/*.patch). I renamed it and pointed the application to the library's patch. Sorry about that.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 6.topic: python Python is a high-level, general-purpose programming language. labels Aug 19, 2025
@johnrichardrinehart
Copy link
Contributor Author

johnrichardrinehart commented Aug 19, 2025

@RossSmyth Thanks for the deep review. I've:

  1. created a PR to fix: add a pyproject.toml to modernize the build system louwrentius/fio-plot#156 and used that commit to pull in my patch and removed my in-tree patch file;
  2. removed with lib; in the various meta sections;
  3. added a pythonImportsCheck for pyan3 (module names itself "pyan", apparently);
  4. removed things like python3Packages.foo and instead moved foo into the args directly;
  5. added a meta.mainProgram to fio-plot;
  6. made sure that it still builds;

I think that's everything. Please let me know if there's more work to do!

@RossSmyth
Copy link
Contributor

Hm taking a bit of a closer look, is the python module fio_plot the same thing as the binary fio-plot except run as a program?

Copy link
Contributor

@RossSmyth RossSmyth left a comment

Choose a reason for hiding this comment

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

I didn't notice this when I was reviewing earlier. Since it is just a different view of the same source you can just have this.

Comment on lines 1 to 43
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah you can just replace the whole thing with this.

Suggested change
{
lib,
fetchFromGitHub,
fio,
python3Packages,
}:
let
fio_plot = python3Packages.fio_plot;
in
python3Packages.buildPythonApplication rec {
pname = "fio-plot";
version = "1.1.16";
pyproject = true;
pythonPath = [
fio_plot
];
patches = fio_plot.patches;
build-system = [ python3Packages.setuptools ];
src = fetchFromGitHub {
owner = "louwrentius";
repo = "fio-plot";
tag = "v${version}";
hash = "sha256-yN0gVm6ZYEIoh91d+0ohJ9yU+VWwYEq3MoG+WgBrs2Q=";
};
dependencies = [
fio
];
meta = {
description = "Create charts from FIO storage benchmark tool output";
homepage = "https://github.com/louwrentius/fio-plot";
license = lib.licenses.bsd3;
mainProgram = "fio-plot";
maintainers = with lib.maintainers; [ johnrichardrinehart ];
};
}
{
python3Packages,
}:
python3Packages.toPythonApplication python3Packages.fio_plot

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I had to overrideAttrs for meta.mainProgram but otherwise I did exactly that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't forget the import check

@RossSmyth
Copy link
Contributor

You also need to squash your commits to follow the commit convention.

@johnrichardrinehart
Copy link
Contributor Author

You also need to squash your commits to follow the commit convention.

Alright. I'll do that if the current state of all the files look good (as of 4558e93e7d269e3750083f03c659637847395294). Let me know and then I'll squash. I kept the history around in case I needed to revert and to support the review.

Copy link
Contributor

@RossSmyth RossSmyth left a comment

Choose a reason for hiding this comment

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

Last thing

Copy link
Contributor

@RossSmyth RossSmyth Aug 20, 2025

Choose a reason for hiding this comment

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

You can check the output with nix eval -f ./. fio-plot.meta

Suggested change
meta.mainProgram = "fio-plot";
meta = old.meta // { mainProgram = "fio-plot"; };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, nice. Changed!

@johnrichardrinehart
Copy link
Contributor Author

Last thing

Squashing.

@RossSmyth
Copy link
Contributor

Oops, for the commit structure it should be three commits:

  1. python3Packages.pyan3: init at v1.2.0
  2. python3Packages.fio_plot: init at 1.1.16
  3. fio-plot: init at 1.1.16

One for each new package added.

@johnrichardrinehart
Copy link
Contributor Author

Oops, for the commit structure it should be three commits:

  1. python3Packages.pyan3: init at v1.2.0
  2. python3Packages.fio_plot: init at 1.1.16
  3. fio-plot: init at 1.1.16

One for each new package added.

Okay, I think we're stable!

Copy link
Contributor

@RossSmyth RossSmyth left a comment

Choose a reason for hiding this comment

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

Nice!

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 20, 2025
Copy link

Choose a reason for hiding this comment

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

Suggested change
description = "Static call graph generator. The official Python 3 version.";
description = "Static call graph generator";

Be short, just one sentence
Not end with a period
https://github.com/NixOS/nixpkgs/tree/master/pkgs#meta-attributes

Copy link

Choose a reason for hiding this comment

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

please add mainProgram

Copy link
Contributor

@RossSmyth RossSmyth Aug 21, 2025

Choose a reason for hiding this comment

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

This is a package. It does not have a mainProgram. At least not how it is used here.

Copy link

Choose a reason for hiding this comment

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

please add pytest

@nixpkgs-ci nixpkgs-ci bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 6.topic: python Python is a high-level, general-purpose programming language. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants