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

brother-dcpl2550dw: init at 4.0.0-1 #321125

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

p4p4j0hn
Copy link

@p4p4j0hn p4p4j0hn commented Jun 19, 2024

Description of changes

This adds the printer driver for the Brother DCPL2550DW.
It combines the cups-wrapper and the driver in one file which allows
sharing common variables.

Things done

Currently using these packages on my laptop.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4292

@p4p4j0hn
Copy link
Author

Result of nixpkgs-review pr 321125 run on x86_64-linux 1

2 packages built:
  • dcpl2550dw-cupswrapper
  • dcpl2550dw-drv

@Yarny0
Copy link
Contributor

Yarny0 commented Jul 20, 2024

Review per https://github.com/NixOS/nixpkgs/blob/b59dfac3c65d7ccc12426527553c9250cb71e90a/pkgs/README.md#new-packages

Thanks for your contribution to nixpkgs!

Reviewed points
  • package path fits guidelines: seems ok if moving to by-name is not an option (see below)
  • package name fits guidelines
  • package version fits guidelines
  • package builds on x86-64
  • executables tested on ARCHITECTURE (not tested)
  • meta.description is set and fits guidelines
  • meta.license fits upstream license: no
  • meta.platforms is set
  • meta.maintainers is set
  • meta.mainProgram is set, if applicable.
  • build time only dependencies are declared in nativeBuildInputs
  • source is fetched using the appropriate function
  • the list of phases is not overridden
  • when a phase (like installPhase) is overridden it starts with runHook preInstall and ends with runHook postInstall.
  • patches have a comment describing either the upstream URL or a reason why the patch wasn't upstreamed
  • patches that are remotely available are fetched rather than vendored
Possible improvements
  • I suggest to resolve most of the constant values (like model and MODEL) as they probably never change within this derivation. The few that might change (like version) can be defined inside the mkDerviation attribute set and then be referenced by a finalAttrs recursion (you can find examples for the finalAttrs pattern in other derivations in nixpkgs).

  • In the driver output, there is a broken symlink in etc. That's probably a typo where this symlink is created.

  • The gpl2 license is deprecated (some info here: treewide: remove licenses.gpl2 #305036 ). The paperconfigml2 and lpdwrapper files in the source archive state ...either version 2 of the License, or (at your option) any later version. So it would be gpl2Plus.

  • The installPhase of the cupswrapper should be indented.

  • There are some message in the build log that might need attention:

    • Can the error dpkg-deb: error: --extract needs a target directory. from the build log be resolved? Maybe the unpackCmd isn't necessary at all, as the dpkg setuphook takes care of unpacking the archive.
    • '--replace' is deprecated, use --replace-{fail,warn,quiet}. for substituteInPlace.
  • There is currently a movement to move away from sha256="..." hashes to hash="sha256-..." SRI hashes: Tracking: deprecate sha256 attribute in fetchers in favor of hash = "<SRI hash>" #325892 . It might be more future-proof to use an SRI hash.

Comments
  • Are you sure that both derivations are independent of each other? The file opt/brother/Printers/DCPL2550DW/cupswrapper/.lpdwrapper-wrapped in the cupswrapper references brprintconflsr3 and lpdfilter and apparently attempts to execute these files as binaries. The driver drivation has executable files with these names, which makes me think that cupswrapper needs driver.
    In that case, the lpdwrapper should be patched so it can find those files, which also makes driver a runtime dependency of cupswrapper. And with this constellation, and given that both derivations already share a lot of build code, it might be better to create one derivation. This would shorten the build recipe a lot. Then you should also move it to by-name. If there is a realistic scenario where one would need the driver, but not the cupswrapper, this derivation could also be structured as a multi-output derivation.

  • I don't know how to use this package (I also don't have the hardware, but even then I probably wouldn't now). I suggest to add some infos, either as comment in the build recipe file, or in meta.longDescription.

@p4p4j0hn p4p4j0hn changed the title pkgs/misc/cups/drivers: init brother dcpl2550dw at 4.0.0-1 brother-dcpl2550dw: init at 4.0.0-1 Jul 21, 2024
pkgs/by-name/br/brother-dcpl2550dw/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/br/brother-dcpl2550dw/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/br/brother-dcpl2550dw/package.nix Outdated Show resolved Hide resolved
let
arches = [ "x86_64" "i686" "armv7l" ];

runtimeDeps = [
Copy link
Member

Choose a reason for hiding this comment

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

We could just inline this

Copy link
Author

Choose a reason for hiding this comment

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

Is there an example you can point me to? Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confident that means to just use the values of these variables in those places where use current use the variables. This means the let .. in block is no longer necessary.

This adds the printer driver and cupswrapper for the Brother DCPL2550DW printer/scanner.

I volunteer for maintaining this one as long as I've got the model around.
@Yarny0
Copy link
Contributor

Yarny0 commented Jul 26, 2024

This looks really good now. I tested it (not with real hardware, but by printing into a file), and the ppd file and the filter produce data without error.

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

Successfully merging this pull request may close these issues.

5 participants