void-packages icon indicating copy to clipboard operation
void-packages copied to clipboard

Add Printer Support: Brother DCP-L8410CDW

Open shahab-vahedi opened this issue 1 year ago • 5 comments

The LPR drivers and CUPS wrappers from Debian packages provided on Brother's Support page.

Testing the changes

  • I tested the changes in this PR: YES

New package

Local build testing

  • I built this PR locally for my native architecture, x86_64-glibc

shahab-vahedi avatar Aug 02 '24 14:08 shahab-vahedi

Ping!

shahab-vahedi avatar Aug 10 '24 19:08 shahab-vahedi

Ping^2!

shahab-vahedi avatar Aug 17 '24 06:08 shahab-vahedi

Ping^3!

shahab-vahedi avatar Aug 25 '24 13:08 shahab-vahedi

Ping^4!

shahab-vahedi avatar Aug 31 '24 17:08 shahab-vahedi

Could someone take a look at these? I've been using them for a month and see no surprises.

shahab-vahedi avatar Sep 09 '24 07:09 shahab-vahedi

I'm not inclined to accept these packages. At best, this ought to be considered for inclusion as a restricted package

I can add restricted=yes to the template. HOWEVER, if the community doesn't want this package, then I keep it to myself. I just wanted to share it with other Void users to save them some time.

The EULAs and wrapper script should not be vendored...

For the EULA, my point of reference was the agree.html already existing in bother-brscan4 package.

Regarding the wrapper script, If you're referring to cupswrapperdcpl8410cdw.void, it's a trimmed down version of the original script in the *.deb file to suit a Void Linux system. The alternative would be to have the original one installed and apply a patch on top of it. For ease of maintenance, I chose the former approach. For the record, the brother-dcp197c-cupswarpper, as it is in master branch of 24-Oct-2024, was my inspiration.

...Fetch them from the deb archive or the web at build time.

I find nothing in the *.deb file. As for the web, I neither am sure where that would be nor have I the will to locate it. Maybe I should just remove the agree.html.

The INSTALL and REMOVE scripts are inappropriate and should be removed. Our packages don't restart users' services just by virtue of installation or removal. Users can restart their own services when appropriate.

This was a mimic of the *.deb package itself, but sure, I can remove them. Keep in mind, one of them, brother-dcp8410-lpr/INSTALL, is creating a directory and has nothing to do with the services.

I appreciate that someone has finally taken a peek at this. Whatever the outcome of this discussion will be, it will inherently be the same for my other PR which upgrades (and fixes) the (broken) existing package of brother-brscan4.

shahab-vahedi avatar Oct 24 '24 19:10 shahab-vahedi

Vendoring the wrapper script entirely just obscures the changes you made, which makes it harder to maintain as the upstream version drifts over time. It also makes your changes harder to review, because we can't see if the patch is appropriate. Use the version from the deb archive and include a patch that makes the necessary changes for the Void package.

We provide a make_dirs variable to allow directories to me made during package installation. That should be preferred over a custom INSTALL script to do the same.

If the brother-brscan4 package venders a EULA, it is doing the wrong thing. The license should be captured at package build time because it is subject to change between the moment you fetched a copy and the moment the package fetched distfiles and built a package. If it isn't in the deb archive, you can grab it at build time from wherever you got it in the first place.

ahesford avatar Oct 25 '24 13:10 ahesford

Vendoring the wrapper script entirely just obscures the changes you made, which makes it harder to maintain as the upstream version drifts over time. It also makes your changes harder to review, because we can't see if the patch is appropriate. Use the version from the deb archive and include a patch that makes the necessary changes for the Void package.

Ack.

We provide a make_dirs variable to allow directories to me made during package installation. That should be preferred over a custom INSTALL script to do the same.

Ack.

If the brother-brscan4 package venders a EULA, it is doing the wrong thing. The license should be captured at package build time because it is subject to change between the moment you fetched a copy and the moment the package fetched distfiles and built a package. If it isn't in the deb archive, you can grab it at build time from wherever you got it in the first place.

Could you name a package that already does something like that? so I can make myself familiar with the whole picture/mechanism.

shahab-vahedi avatar Oct 25 '24 17:10 shahab-vahedi

If the brother-brscan4 package venders a EULA, it is doing the wrong thing. The license should be captured at package build time because it is subject to change between the moment you fetched a copy and the moment the package fetched distfiles and built a package. If it isn't in the deb archive, you can grab it at build time from wherever you got it in the first place.

Could you name a package that already does something like that? so I can make myself familiar with the whole picture/mechanism.

The post_extract() in brother-brscan3 package does that exactly.

shahab-vahedi avatar Oct 28 '24 18:10 shahab-vahedi

@ahesford ChangeLog for the new changes:

brother-dcp8410-lpr

  • Removed README.voidlinux As a consequence, INSTALL and REMOVE scripts are added to perform the actions mentioned in there.

  • Added a mechanism to get an up-to-date ToS from vendor's website. Also verify if the checksum of the agreement has remained the same.

  • Did NOT mark the package as restricted, because ToS allows redistribution.

brother-dcp8410-cups

  • Removed INSTALL and REMOVE scripts

  • Renamed README.void to README.maintainer I really hate to see this doc go away. It is not documented at the vendor website. I've gathered those notes while studying the wrapper script.

  • Replaced the cupswrapperdcpl8410cdw.void script with a patch The patch is 328 lines while the drop-in script was only 152 lines. I believe it is easier to wrap one's head around the drop-in wrapper script (pun intended) rather than going through the 300+ lines of patch. Specially that the upstream package hasn't been updated for years, there isn't virtually any maintenance work to update the drop-in wrapper.

  • Added a mechanism to get an up-to-date ToS from vendor's website. Also verify if the checksum of the agreement has remained the same.

  • Did NOT mark the package as restricted, because ToS allows redistribution.

shahab-vahedi avatar Oct 28 '24 20:10 shahab-vahedi

I think you misinterpreted my comments:

  • Marking the package restricted is a precondition for consideration, without consideration for terms of service that may allow redistribution. It should be restricted because the build process is fragile. A precompiled deb offered by a corporate sponsor may disappear at any time, or may break and be unfixable becuase of other Void updates. We do not need to accept the possibility that officially repacking this software may break our repository at an inopportune moment.
  • The README.maintainer does not belong in our repository. As I noted, users are expected to familiarize themselves with the maintenance of printers in CUPS should they wish to use it. Void is not a repository for general-purpose documentation.
  • The INSTALL and REMOVE scripts remain inappropriate. Void does not alter general system configuration just by virtue of package installation or removal. Removing or adding printers when this package is installed or removed is not appropriate; users may wish to install or remove packages without rewriting printcap. This is especially true because the -lpr package is a dependency of the -cups package, but CUPS tends to own /etc/printcap; other packages shouldn't be modifying that file at all.

In addition:

  • In the -lpr package, why are you rearranging the executable structure? If the package installs executables in x86_64 and i686 subdirectories, you should leave them where they are, but remove the unnecessary arch for each of the i686 and x86_64 packages.
  • The wrapper script in -cups is poorly written (it doesn't even spell lpdwrapper correctly) and isn't really suitable for Void at all. The script should never copy files into the /usr/share or /usr/lib* trees, because those are owned by the package manager. The attempt to start, stop or restart services is also poorly conceived. The Void template should just symlink the PPD file(s) as appropriate, so that the package manager can manage those links.
  • There is no need for curl or fetching the license file at runtime; you can just add the EULA URL to the distfiles array in each case, using the following construct:
    distfiles="<regular-distfile>
     https://support.brother.com/g/s/agreement/English_lpr/agree.html>eula-${version}.html"
    skip_extraction="eula-${version}.html"
    
    post_extract() {
        sed -n -e <...> "${XBPS_SRCDISTDIR}/${pkgname}-${version}/eula-${version}.html" > LICENSE
    }
    

ahesford avatar Oct 29 '24 12:10 ahesford

Pull Requests become stale 90 days after last activity and are closed 14 days after that. If this pull request is still relevant bump it or assign it.

github-actions[bot] avatar Jan 28 '25 01:01 github-actions[bot]