media-gfx/sane-backends-1.0.28 #14330
Conversation
Pull Request assignmentSubmitter: @aclex media-gfx/sane-backends: @gentoo/proxy-maint (maintainer needed) Linked bugsNo bugs to link found. If your pull request references any of the Gentoo bug reports, please add appropriate GLEP 66 tags to the commit message and request reassignment. In order to force reassignment and/or bug reference scan, please append Docs: Code of Conduct ● Copyright policy (expl.) ● Devmanual ● GitHub PRs ● Proxy-maint guide |
gentoo-bot
added
maintainer-needed
|
|
||
| src_unpack() { | ||
| unpack ${A} | ||
| mv -v ${WORKDIR}/backends-${PV} ${S} |
There was a problem hiding this comment.
Yes, sure. Thank you for pointing out!
|
@heroxbd Thank you very much for the review, Benda! Have just done this little fix. Another question worries me here is the failed check on separate user and group for |
|
I think it has better be done now. As you see this |
|
@heroxbd yes, I'd also prefer to fix it, but not sure I'll be able to maintain it in the long term. Could you please just suggest, what is the process of getting approved UID and GID for sane? I hope, this part is something I'm able to do now. |
|
https://projects.gentoo.org/qa/policy-guide/user-group.html It's explained here. Just basically pick the next free number going from 499 downwards. Then whoever commits your work, will pick the next available if someone else already took what you suggested. Quite simple. |
| @@ -0,0 +1,340 @@ | |||
| # Copyright 1999-2019 Gentoo Authors | |||
There was a problem hiding this comment.
Please also update the copyright year next time you commit for this package, or use repoman to commit so it fixes this for you automatically.
| EAPI=7 | ||
| inherit flag-o-matic multilib-minimal systemd toolchain-funcs udev user |
There was a problem hiding this comment.
One empty line below EAPI please.
|
|
||
| for backend in ${IUSE_SANE_BACKENDS}; do | ||
| case ${backend} in | ||
| # Disable backends that require parallel ports as no one has those anymore. |
There was a problem hiding this comment.
If these are disabled, why are they even offered to be chosen anyway? I feel like package.use.mask would work better here.
There was a problem hiding this comment.
I'm afraid, this is beyond me to write correctly. Could you please suggest more straightforward snippet for this? Would it be fine enough, if I just remove the case labels from IUSE_SANE_BACKENDS? Or have you meant mentioning them in package.use.mask in metadata.xml?
There was a problem hiding this comment.
Yeah, that's what I meant. It makes no sense to have them in IUSE if they cannot be controlled in the ebuild. Just write them manually in econf to let people know the option is there.
There was a problem hiding this comment.
Nice, thank you, I'll try to make it that way and check the build.
There was a problem hiding this comment.
I've thought on this looking for the best way of solving, and, well, "no one has parallel ports nowadays" sounds not enough convincing to me to remove the working backends… Maybe that's me about to install internal FDD soon :) But I've chosen to move these backends on the next line (i.e. keep them available for enabling, without +). I've checked them myself, compiles fine, so who knows, maybe someone would just need this functionality and would be happy it's still there. Please, correct me if I'm wrong here.
|
|
||
| LICENSE="GPL-2 public-domain" | ||
| SLOT="0" | ||
| KEYWORDS="alpha amd64 arm arm64 hppa ia64 ~m68k ~mips ppc ppc64 ~s390 ~sh sparc x86 ~amd64-linux ~x86-linux" |
There was a problem hiding this comment.
You can not commit new versions straight to stable. Use ekeyword ~all sane-backends-1.0.28.ebuild to reset keywords for this before committing.
There was a problem hiding this comment.
Yes, indeed, overseen this. Thanks for pointing out! Fixed now.
| >=virtual/jpeg-0-r2:0=[${MULTILIB_USEDEP}] | ||
| >=media-libs/tiff-3.9.7-r1:0=[${MULTILIB_USEDEP}] |
| HOMEPAGE="http://www.sane-project.org/" | ||
| MY_P="${P}" | ||
| FRS_ID="4224" | ||
| SRC_URI="https://gitlab.com/sane-project/backends/uploads/9e718daff347826f4cfe21126c8d5091/sane-backends-${PV}.tar.gz" |
There was a problem hiding this comment.
Don't they have a tagged release?
There was a problem hiding this comment.
Yes, haven't used it for some reason, changed now.
There was a problem hiding this comment.
Unfortunately, I've just found, that I've been confused by the old archive in my cache, which pretended to be right. The archive with source code auto-generated by GitLab out there is not consistent, in spite of the nice direct link. It needs autoreconf to be done, which in turn relies on the target being a git repository, which is not the case (quite common story for auto-generated by git archive archives trying to pull its version from git). So, autotools eclass fails to prepare it and, I'm afraid, I'm end up reverting it back to this obfuscated link (the uploaded archive actually).
| DESCRIPTION="Scanner Access Now Easy - Backends" | ||
| HOMEPAGE="http://www.sane-project.org/" | ||
| MY_P="${P}" | ||
| FRS_ID="4224" |
There was a problem hiding this comment.
Some variables for the previous URL, removed now.
| # if LINGUAS is set, just use the listed and supported localizations. | ||
| if [[ ${LINGUAS+set} == "set" ]]; then | ||
| mkdir -p po || die | ||
| strip-linguas -u po | ||
| printf '%s\n' ${LINGUAS} > po/LINGUAS | ||
| fi |
There was a problem hiding this comment.
We just usually install all language files unconditionally. Users need to utilize INSTALL_MASK to prevent unwanted ones from being installed.
There was a problem hiding this comment.
So can I just remove this block completely here to prevent this filtering?
There was a problem hiding this comment.
Make sure it still installs the files, but yes, I think you can do that.
There was a problem hiding this comment.
OK, thank you! Will try to remove it as well.
There was a problem hiding this comment.
Removed in the last commit, all the locales now seem to be installed properly.
|
|
||
| multilib_src_configure() { | ||
| # the blank is intended - an empty string would result in building ALL backends. | ||
| local BACKENDS=" " |
There was a problem hiding this comment.
local variables are written with lowercase lettering to distinct them from global ones.
| mkdir -p "${dirs[@]}" || die | ||
| emake "${targets[@]}" | ||
|
|
||
| popd >/dev/null |
There was a problem hiding this comment.
Yes, fixed in the last line (if I'm not wrong, emake doesn't require this).
|
@juippis thank you so much for suggestion concerning user and group ID number obtaining and all the comments in the review! Though I've just copied an existing ebuild, I'll try to fix as much, as I'd be able to, since we're still putting hands on it now. |
|
@juippis I've pushed the fixes for the main part of the issues, just a couple of them remaining and the one with UID/GID. Will look at it as soon, as possible, too. |
|
@juippis So fortunately I seem to manage implementing GLEP 81, so there're only an issue with I've also noticed, there's another version released, 1.0.29, so I've decided to push the ebuild for it as well. I've tested the new functionality on this, it compiles successfully. I'm not sure if it's a good idea to have two ebuilds in the same request, but maybe you'd suggest to leave only the latest one, so I've finally decided to add it. |
aclex
force-pushed
the
sane-backends-1.0.28
branch
from
cc249e6
to
a2946a5
Compare
You should probably just have the latest, since if you push 2 different versions with same KEYWORDS to tree, people will always have the later one installed. Good work so far, I'll start testing this soon! |
|
@juippis thanks for taking a look at the changes and my questions! I'll then try to close the remaining issues and remove the previous version now to make it as complete, as possible. |
aclex
force-pushed
the
sane-backends-1.0.28
branch
from
786872e
to
88bdfde
Compare
There was a problem hiding this comment.
Looks good, and builds fine, but I need to ask you to clean up and split the commits a bit. You should have 3 commits in total here,
1st: add acct-group/scanner group,
2nd: add acct-user/saned ebuild,
3rd: bump sane-backends to 1.0.29 with the sane_backends.desc change as well.
EDIT: Also you should use your commit message syntax, described in
https://www.gentoo.org/glep/glep-0066.html#commit-messages
aclex
force-pushed
the
sane-backends-1.0.28
branch
2 times, most recently
from
07bff60
to
4f720e9
Compare
|
Thanks, updated it, hope it's now fine. |
|
You need 3 commits instead of 2. You also need to add metadata.xml files with commits, I think they were there but got lost with rebase or something? I think the pull request history will allow you to dig these files, but I'm not sure. |
Add `acct-group/scanner` for SANE backend server to use instead of previous `user.class` instructions to be compatible with GPEP 81. Signed-off-by: Alexey Chernov <4ernov@gmail.com>
Add `acct-user/saned` for SANE backend server to use instead of previous `user.class` instructions to be compatible with GPEP 81. Signed-off-by: Alexey Chernov <4ernov@gmail.com>
Also make the updated ebuild GLEP 81 compliant, add the necessary description for the new backend appeared, ix some issues suggested during the review process. Signed-off-by: Alexey Chernov <4ernov@gmail.com>
aclex
force-pushed
the
sane-backends-1.0.28
branch
from
4f720e9
to
d08209f
Compare
|
Fixed (hopefully). |
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <!DOCTYPE pkgmetadata SYSTEM "http://www.gentoo.org/dtd/metadata.dtd"> | ||
| <pkgmetadata> | ||
| <!-- maintainer-needed --> |
There was a problem hiding this comment.
Also I'm sorry but we can not add new packages without maintainer, it's against the policy.
https://projects.gentoo.org/qa/policy-guide/maintainer.html#pg0602
Didn't you originally want to become the proxy-maintainer for all of these packages?
There was a problem hiding this comment.
No, I didn't. Initially I just wanted to submit a version bump, as it increases the number of supported devices (including mine). Then @heroxbd suggested me to fix the inconsistencies using this case changes. Maintaining it is a little too much for me, especially given how long and difficult this process of adding simple version bump became.
Let's then leave it here in case of anyone would be interested.
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <!DOCTYPE pkgmetadata SYSTEM "http://www.gentoo.org/dtd/metadata.dtd"> | ||
| <pkgmetadata> | ||
| <!-- maintainer-needed --> |
|
2359950 it was fastly bumped due to security vulnerabilities. I incorporated your changes afterwards, so your work didn't fully go to vain here. |
|
@juippis oh, nice, thank you! Also thanks for informing me here. |
Just an instance of version 1.0.27 ebuild with necessary fixes in terms of patches (all seen and ensured to be necessary/applicable or not) and source code URL for recently released 1.0.28. Among other things, it adds support for some new devices.