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

media-video/motion: version bump to 4.3.1 #15513

Closed

Conversation

hfernh
Copy link
Contributor

@hfernh hfernh commented Apr 25, 2020

New version of motion, which fixes
Bug: https://bugs.gentoo.org/665930
Bug: https://bugs.gentoo.org/673410
Bug: https://bugs.gentoo.org/717312

Signed-off-by: Johannes Willem Fernhout hfern@fernhout.info

@gentoo-bot gentoo-bot added assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels Apr 25, 2020
@hfernh
Copy link
Contributor Author

hfernh commented Apr 25, 2020

Copy link
Contributor

@vilhelmgray vilhelmgray left a comment

Choose a reason for hiding this comment

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

Squash this patch into your previous one so that it's a single commit that can be merged when it's approved.

@vilhelmgray
Copy link
Contributor

vilhelmgray commented Apr 25, 2020

references to bugs:
https://bugs.gentoo.org/665930
https://bugs.gentoo.org/673410
https://bugs.gentoo.org/717312

Change the Github pull request title at the top of this page and append [please reassign] to the end. This will cause the bot to automatically recheck the commit messages for these bug links.

@vilhelmgray
Copy link
Contributor

vilhelmgray commented Apr 25, 2020

I reviewed the bot's QA issue list:

1. DeprecatedEclass / uses deprecated eclass: [ user (migrate to acct-user/acct-group packages) ]
   => removed reference to user

You need to create a motion user package and a motion group package. Add them as dependencies to your motion ebuild. Here's some more information about the format: https://wiki.gentoo.org/wiki/Categories_acct-group_and_acct-user

2. UnknownUseFlags | unknown USE flag: 'mariadb'
   => not exactly sure what caused this, maybe an extra space behind mariadb in the IUSE string..?

Add a "mariadb" entry in the metadata.xml file: 91f6eee#diff-f19779b8008ab61e7ff7cf996b971d96

@hfernh
Copy link
Contributor Author

hfernh commented Apr 26, 2020

Have reserved a motion uid/gid in a pull request gentoo/api-gentoo-org#272 . Previously I had removed the motion user from the ebuild, but once this pull request 272 is approved I will reinstate the motion user, and process the other comments.

@vilhelmgray
Copy link
Contributor

vilhelmgray commented May 3, 2020

This is looking better, but squash all those commits down to just one.

The [please reassign] needs to be appended to the title of this Github pull request, not the body. There's an Edit button to the right of the title that you can click to change it (try Ctrl+F to search "Edit" for it if you're having trouble finding it).

I also recommend applying to become a Proxy Maintainer for this package. That will probably help expedite the process to merge your pull request.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-05-03 18:17 UTC
Newest commit scanned: 8bcb255
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/a7cfb89/output.html

@hfernh hfernh changed the title media_video/motion: version bump to 4.3.1 media_video/motion: version bump to 4.3.1 [please reassign] May 4, 2020
@gentoo-bot gentoo-bot changed the title media_video/motion: version bump to 4.3.1 [please reassign] media_video/motion: version bump to 4.3.1 May 4, 2020
@gentoo-bot gentoo-bot added new package The PR is adding a new package. assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. no signoff One or more commits do not indicate GCO sign-off. and removed assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. labels May 4, 2020
@hfernh hfernh force-pushed the media_video/motion-4.3.1-version-bump branch 2 times, most recently from d93c61f to 89aa84f Compare May 4, 2020 12:27
@hfernh hfernh changed the title media_video/motion: version bump to 4.3.1 media_video/motion: version bump to 4.3.1 [please reassign] May 4, 2020
@gentoo-bot gentoo-bot changed the title media_video/motion: version bump to 4.3.1 [please reassign] media_video/motion: version bump to 4.3.1 May 4, 2020
@gentoo-bot gentoo-bot added new package The PR is adding a new package. assigned PR successfully assigned to the package maintainer(s). no bug found No Bug/Closes found in the commits. and removed assigned PR successfully assigned to the package maintainer(s). new package The PR is adding a new package. no bug found No Bug/Closes found in the commits. no signoff One or more commits do not indicate GCO sign-off. labels May 4, 2020
@gentoo-bot gentoo-bot added new package The PR is adding a new package. assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. and removed assigned PR successfully assigned to the package maintainer(s). new package The PR is adding a new package. no bug found No Bug/Closes found in the commits. no signoff One or more commits do not indicate GCO sign-off. labels May 5, 2020
@hfernh
Copy link
Contributor Author

hfernh commented May 5, 2020

I think I finally managed to squash the commits together, get the right labels and bug references added.

@hfernh
Copy link
Contributor Author

hfernh commented May 5, 2020

@juippis based on the earlier suggestion from @vilhelmgray to volunteer as proxy-maintainer I contacted @mgorny, who suggested that you should have a look at this PR.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-05-06 09:04 UTC
Newest commit scanned: 70947f7
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/50fd808/output.html

Copy link
Member

@juippis juippis left a comment

Choose a reason for hiding this comment

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

You need to split this into 3 commits. One for acct-user, one for acct-group and one for media-video/motion.
Your commit summary needs to be updated, it's media**-**video, not media_video. And you can update your Bug: tag to be Closes: tag so it closes the bugs automatically when this is merged. Overall it looks good, just few small things here and there.

Comment on lines 3 to 6
<pkgmetadata>
<maintainer type="project">
<email>media-video@gentoo.org</email>
</maintainer>
Copy link
Member

Choose a reason for hiding this comment

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

You can't add them here without their consent. Did you ask? If not, put yourself and proxy-maint project here.
https://wiki.gentoo.org/wiki/Project:Proxy_Maintainers/User_Guide#Proxied_maintainer_in_metadata.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, have not asked them; I assumed because they are the maintainer of media-video/motion that they'd also maintain the uid/gid. I will change the maintainer to me and the proxy maint group.


DESCRIPTION="added by portage for motion, a software motion detector"
ACCT_USER_ID=395
ACCT_USER_HOME=/var/lib/motion
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason for a home dir is that motion writes it's output files in the current working directory (or the directory defined in it's config file). Looking at Michael Orlizky's article I should create the home dir from the motion-video/motion ebuild though, not assume it'll be created by acct-user/motion ebuild.

Comment on lines +5 to +9
#MOTION_USER="motion"
#MOTION_GROUP="motion"
Copy link
Member

Choose a reason for hiding this comment

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

Why is everything commented in this file? Looks ok to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All the comments are just documentation, showing default values.

@@ -0,0 +1,16 @@
#!/sbin/openrc-run
# Copyright 1999-2017 Gentoo Foundation
Copy link
Member

Choose a reason for hiding this comment

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

2020 and switch Foundation to Authors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will do

Group=video

#RuntimeDirectory=motion
#RuntimeDirectoryMode=0750

WorkingDirectory=/var/lib/motion
#WorkingDirectory=/var/lib/motion
Copy link
Member

Choose a reason for hiding this comment

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

Why comment? Just remove if it's not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I should actually make a separate r4 version and leave the original as is for motion-4.1.1. That way existing 4.1.1 won't be bothered with any changes and 4.1.1 can be deprecated over time. The new version can set the uid/gid and the working dir properly.

Comment on lines 4 to 6
EAPI=7
inherit autotools readme.gentoo-r1 systemd
Copy link
Member

Choose a reason for hiding this comment

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

One empty line below EAPI please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

inherit autotools readme.gentoo-r1 systemd

DESCRIPTION="A software motion detector"
HOMEPAGE="https://motion-project.github.io"
Copy link
Member

Choose a reason for hiding this comment

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

https://

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you want me to do with this...

Comment on lines 20 to 23
ffmpeg? (
libav? ( media-video/libav:= )
!libav? ( media-video/ffmpeg:0= )
)
Copy link
Member

Choose a reason for hiding this comment

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

libav has been removed from ::gentoo tree.

Copy link
Contributor Author

@hfernh hfernh May 7, 2020

Choose a reason for hiding this comment

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

Ok, I'll change it to ffmpeg? ( media-video/ffmpeg:0= )

( use mysql || use mariadb ) || sed -i -e "/use mysql/d" "${D}/etc/init.d/motion"
use postgres || sed -i -e "/use postgresql/d" "${D}/etc/init.d/motion"
( use mysql || use mariadb || use postgres ) || sed -i -e "/depend/{N;N;d;}" "${D}/etc/init.d/motion"
Copy link
Member

Choose a reason for hiding this comment

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

What if you did this / constructed the init.d file before calling newinitd?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That might work too: I would change "${D}/etc/init.d/motion" to "${FILESDIR}"/${PN}.confd-r4" in these three lines.

Why would that be better?

Copy link
Member

Choose a reason for hiding this comment

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

Because this is hard to make sense at. And all external commands need to die (sed). So you'll have to open this into if ... then;

You can use something like https://github.com/gentoo/gentoo/blob/master/mail-client/thunderbird-bin/thunderbird-bin-68.8.0.ebuild#L151 with newinitd.

use supervise-daemon && echo "supervisor=\"supervise-daemon\"" >> "${D}/etc/init.d/motion"
use supervise-daemon || echo "pidfile=\"/run/\${RC_SVCNAME}.pid\"" >> "${D}/etc/init.d/motion"
use supervise-daemon || echo "command_background=true" >> "${D}/etc/init.d/motion"
Copy link
Member

Choose a reason for hiding this comment

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

This is doable with a single echo. Although I'd say to construct this file with newinitd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing with a single echo, using echo -e, and adding a \n for the newline but that added unwanted spaces. An alternative would be using a heredoc, but that won't be shorter nor easier to read, I think.

Not sure how to construct a file using newinitd, other then have a separate initd file for this, which would make us maintain more files.

@hfernh hfernh force-pushed the media_video/motion-4.3.1-version-bump branch from 70947f7 to fb2d6ba Compare May 11, 2020 17:03
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-05-11 17:50 UTC
Newest commit scanned: fb2d6ba
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/4e78e22419/output.html

@hfernh
Copy link
Contributor Author

hfernh commented May 11, 2020

I think I resolved all issues. However the bot checked only the last commit. When @juippis wrote:

You need to split this into 3 commits. One for acct-user, one for acct-group and one for media-video/motion.

Di you mean three pull separate pull requests?

@juippis
Copy link
Member

juippis commented May 12, 2020

Di you mean three pull separate pull requests?

No, it looks good now. 3 commits 1 PR.

Summary of changes:
- support new version 4.3.1
- EAPI6 to EAPI7
- GLEP81 support: user and group management via dedicated packages
- untangled support for mariadb and mysql
- remove libav
- confd support for commandline options, umask and work dir
- initd checkpre function to check existence of work dir and log dir
- support for OpenRC's supervise-daemon via a local use flag
- added myself as proxy-maintainer

Closes: https://bugs.gentoo.org/665930
Closes: https://bugs.gentoo.org/673410
Closes: https://bugs.gentoo.org/717312

Signed-off-by: Johannes Willem Fernhout <hfern@fernhout.info>
New group for media-video/motion, gid 395.
See also gentoo/api-gentoo-org#272.

Signed-off-by: Johannes Willem Fernhout <hfern@fernhout.info>
New user for media-video/motion, uid 395.
See also gentoo/api-gentoo-org#272.

Signed-off-by: Johannes Willem Fernhout <hfern@fernhout.info>
@hfernh hfernh force-pushed the media_video/motion-4.3.1-version-bump branch from fb2d6ba to a1c6847 Compare May 15, 2020 16:15
@hfernh hfernh changed the title media_video/motion: version bump to 4.3.1 media-video/motion: version bump to 4.3.1 May 15, 2020
@hfernh
Copy link
Contributor Author

hfernh commented May 15, 2020

I have changed the editing of the initd file by creating just one multi line string with additions (depend function, supervise-daemon/ssd stuff) that should be added. Since I get permission errors when I try to update the source file (file/motion-initd-r4) I now copy it first to a temp file, then add the string, then install the initd file.

@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2020-05-15 16:51 UTC
Newest commit scanned: a1c6847
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/c61020b4e8/output.html

@hfernh
Copy link
Contributor Author

hfernh commented May 23, 2020

Please let me know if there is anything else that needs to be done to get this merged. Thanks.

@hfernh
Copy link
Contributor Author

hfernh commented Jun 1, 2020

@juippis Please let me know if there is anything else that needs to be done to get this merged. Thanks.

Copy link
Member

@juippis juippis left a comment

Choose a reason for hiding this comment

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

Hey,

sorry for ignoring this so long. I'm preparing for a law case which takes all my energy.

So the init.d construction is not what I had in mind but I guess it works. However this needs to be fixed before merging:

 * The ebuild is installing to one or more unexpected paths:
 * 
 *   /usr/local
 * 
 * Please fix the ebuild to use correct FHS/Gentoo policy paths.
 >: equery f motion | grep /local/
/usr/local/bin
/usr/local/bin/motion
/usr/local/etc
/usr/local/etc/motion
/usr/local/etc/motion/camera1-dist.conf
/usr/local/etc/motion/camera2-dist.conf
/usr/local/etc/motion/camera3-dist.conf
/usr/local/etc/motion/camera4-dist.conf
/usr/local/etc/motion/motion-dist.conf
/usr/local/share
/usr/local/share/locale
/usr/local/share/locale/fi
/usr/local/share/locale/fi/LC_MESSAGES
/usr/local/share/locale/fi/LC_MESSAGES/motion.mo
/usr/local/share/locale/sv
/usr/local/share/locale/sv/LC_MESSAGES
/usr/local/share/locale/sv/LC_MESSAGES/motion.mo
/usr/local/share/man
/usr/local/share/man/man1
/usr/local/share/man/man1/motion.1

We don't install anything to /usr/local through portage.

# }
#
# supervisor=supervise-daemon"
local INITDADJ=""
Copy link
Member

Choose a reason for hiding this comment

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

Also while at it, please make a local variable smallcase. We distinct them from global variables this way. Global variables cannot be changed inside a phase while local can.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is weird that is shows installing in /usr/local on your machine. I see this:

$ equery f motion

  • Searching for motion ...
  • Contents of media-video/motion-4.3.1:
    /etc
    /etc/conf.d
    /etc/conf.d/motion
    /etc/init.d
    /etc/init.d/motion
    /etc/motion
    /etc/motion/camera1-dist.conf
    /etc/motion/camera2-dist.conf
    /etc/motion/camera3-dist.conf
    /etc/motion/camera4-dist.conf
    /etc/motion/motion-dist.conf
    /lib
    /lib/systemd
    /lib/systemd/system
    /lib/systemd/system/motion.service
    /usr
    /usr/bin
    /usr/bin/motion
    /usr/share
    /usr/share/doc
    /usr/share/doc/motion-4.3.1
    /usr/share/doc/motion-4.3.1/COPYING.bz2
    /usr/share/doc/motion-4.3.1/CREDITS.bz2
    /usr/share/doc/motion-4.3.1/README.gentoo.bz2
    /usr/share/doc/motion-4.3.1/mask1.png
    /usr/share/doc/motion-4.3.1/motion_build.html
    /usr/share/doc/motion-4.3.1/motion_config.html
    /usr/share/doc/motion-4.3.1/motion_guide.html
    /usr/share/doc/motion-4.3.1/motion_stylesheet.css
    /usr/share/doc/motion-4.3.1/normal.jpg
    /usr/share/doc/motion-4.3.1/outputmotion1.jpg
    /usr/share/doc/motion-4.3.1/outputnormal1.jpg
    /usr/share/locale
    /usr/share/locale/de
    /usr/share/locale/de/LC_MESSAGES
    /usr/share/locale/de/LC_MESSAGES/motion.mo
    /usr/share/locale/nl
    /usr/share/locale/nl/LC_MESSAGES
    /usr/share/locale/nl/LC_MESSAGES/motion.mo
    /usr/share/man
    /usr/share/man/man1
    /usr/share/man/man1/motion.1.bz2

Copy link
Member

Choose a reason for hiding this comment

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

Happens with FEATURES="test".

Copy link
Member

@juippis juippis left a comment

Choose a reason for hiding this comment

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

I'll file a bug about that test failure, and restrict them for now. Thanks and sorry it took this long!

# }
#
# supervisor=supervise-daemon"
local INITDADJ=""
Copy link
Member

Choose a reason for hiding this comment

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

Happens with FEATURES="test".

@hfernh
Copy link
Contributor Author

hfernh commented Jun 4, 2020

Thanks for taking the effort to check. I'll lowercase local INITDADJ and send that PR later today. Please also look at gentoo/api-gentoo-org#272 , which has the UID/GID for motion. Thanks.

@juippis
Copy link
Member

juippis commented Jun 4, 2020

58c1002

@juippis juippis closed this Jun 4, 2020
@hfernh hfernh deleted the media_video/motion-4.3.1-version-bump branch November 2, 2020 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. new package The PR is adding a new package.
Projects
None yet
5 participants