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

PR #2258 with conflicts conservatively resolved #2318

Closed
wants to merge 9 commits into from
Closed

Conversation

MichaIng
Copy link
Member

@MichaIng MichaIng commented Mar 9, 2022

@Mictronics
I rebased your PR and resolved conflicts in a way to prefer the changes that have been done to dev. There isn't much left anymore. I know you did some other changes not directly related to Python 3. Could you have a look and maybe re-add them here? Things I found interesting:

  • Using /%(camera_id)/stream for getting the IP camera stream, which is at least something which seems required for mjpg-streamer, but not sure whether/how it was intended to work before.
  • In handlers you switched from videodevice to video_device motion.conf setting... Ah now I see this is indeed correct for recent motion versions, so we should re-add this change: https://motion-project.github.io/motion_config.html
    EDIT: Done!

Please see whether there were other important changes you want to have re-added.

@MichaIng MichaIng added the python update python 2 > python 3 label Mar 9, 2022
@MichaIng MichaIng mentioned this pull request Mar 9, 2022
@MichaIng
Copy link
Member Author

About the motion settings: I think we need to support both variants, based on the motion version, i.e. the old names for motion <=4.3 and the new variants for motion >=4.4. Is there a good way to check for the motion version?

@MichaIng MichaIng requested review from jmichault and cclauss March 11, 2022 15:05
cclauss
cclauss previously approved these changes Mar 11, 2022
Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

RSLGMT

motioneye/mediafiles.py Outdated Show resolved Hide resolved
cclauss
cclauss previously approved these changes Mar 11, 2022
Copy link
Member

@cclauss cclauss left a comment

Choose a reason for hiding this comment

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

RSLGTM

@MichaIng
Copy link
Member Author

MichaIng commented Mar 11, 2022

I reverted the motion config key changes. We should do this properly in a dedicated PR where we handle support for both versions. With the old/current keys we support motion up to v4.3, hence all versions shipped by distros, while we do not support latest upstream v4.4. The other changes are however pretty fine. Compared to #2258 I did some minor change to extra/ file names and paths to keep them as they were before. Similarly, if possible, we should keep the original behaviour that these extra files (default configs, service) are installed to /usr/local/share/motioneye when the module is installed as root, so that generally non-required changes are minimal.

@cclauss
Can you please review again? I made it so that pushes dismiss existing reviews. If this is to annoying, I can change it back so that approval remain despite new pushes.

@MichaIng
Copy link
Member Author

MichaIng commented Mar 12, 2022

I found the original PRs of two of the major changes here: #2152 and #2110. I'd prefer to merge those (done with one already). The remaining change here is the added sample motion configs, but I wonder what the benefit of them is, since motionEye creates default motion configs internally, and this hence would require doubled code changes, and hardcodes the configs for specific motion versions.

@MichaIng MichaIng changed the title PR #2258 with conservative conflicts resolved PR #2258 with conflicts conservatively resolved Mar 13, 2022
Mictronics and others added 9 commits March 13, 2022 16:56
and use a more generic motionEye scripts path, which matches the one of the current release, when installing as root. We should try to keep this valid and generally changes to install/setup steps low.

Signed-off-by: MichaIng <micha@dietpi.com>
@MichaIng MichaIng removed the request for review from cclauss March 13, 2022 16:06
@MichaIng MichaIng removed the request for review from jmichault March 13, 2022 16:06
@MichaIng MichaIng removed the python update python 2 > python 3 label Mar 13, 2022
@MichaIng
Copy link
Member Author

Marking this as closed in favour if the original PRs. Whether or not default motion configs are wanted, should be discussed in a separate issue/PR.

@MichaIng MichaIng closed this Mar 13, 2022
@MichaIng MichaIng deleted the mictronics branch March 13, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants