Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
Review: Approve -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356881 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
Review: Approve -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356881 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
@trb143 I just checked your media_state branch for the mentioned pymediainfo change. You did not include https://bazaar.launchpad.net/~bastian-germann/openlp/setup/revision/2860. It took me some debugging (no error thrown) to find this. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356881 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
> Please removed all pymediainfo changes as the media code is been extensively > refactored and this will cause issues when that is merged. Your changes have > been taken on board but implemented differently. Did you get the latest utf8 decoding change in 2860? I will remove the changes in the next merge proposal. > Why has run_openlp been restructured instead of just being renamed? Because setuptools' entry_points needs a function. > The license file for fonts will be removed at some point as that icon file > will need to be cleaned up before the release. We are awaiting a dev to come > and help with the font UX work. The fontawsome icons cannot be packaged by > use. The only icons are the 10 we have created from scratch. That is fine. I just added it because the license says it is void if it the license file is not distributed with the font. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356868 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
Review: Needs Fixing Please removed all pymediainfo changes as the media code is been extensively refactored and this will cause issues when that is merged. Your changes have been taken on board but implemented differently. Why has run_openlp been restructured instead of just being renamed? The license file for fonts will be removed at some point as that icon file will need to be cleaned up before the release. We are awaiting a dev to come and help with the font UX work. The fontawsome icons cannot be packaged by use. The only icons are the 10 we have created from scratch. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356868 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
Review: Needs Fixing I highlighted some minor things with diff-comments. Also you should not add more changes to this merge requests: It is starting to become to broad. Diff comments: > === renamed file 'openlp.py' => 'openlp/__main__.py' As mentioned in mailinglist thread linked to above, it was agreed that the renamed file should be "run_openlp.py", please do that instead of __main__.py > > === modified file 'tests/README.txt' > --- tests/README.txt 2012-12-05 18:52:31 + > +++ tests/README.txt 2018-10-16 19:07:47 + > @@ -19,16 +19,16 @@ > > To run the tests, navigate to the root directory of the OpenLP project, and > then run the following command:: > > -nosetests -v tests > +nose2 -v tests > > Or, to run only the functional tests, run the following command:: > > -nosetests -v tests/functional > +nose2 -v tests.functional > > Or, to run only a particular test suite within a file, run the following > command:: > > -nosetests -v tests/functional/test_applocation.py > +nose2 -v tests.functional.openlp_core.test_app > > Finally, to only run a particular test, run the following command:: > > -nosetests -v > tests/functional/test_applocation.py:TestAppLocation.get_frozen_path_test > +nose2 -v > tests.functional.openlp_core.test_app.TestOpenLP.test_process_events pytest is actually the preferred test runner now, so if updating the examples they should should use pytest. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356801 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
That is right. I tested on Linux and Windows. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356675 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
Review: Needs Information Which platforms have you tested this on? If I understand correctly the changes would effect both Linux, Windows and Mac OS. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356675 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
@raoul-snyman Can you add python3-appdirs and python3-pymediainfo to the build machine or point me to where I can do it? -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356602 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
All of my contributions can be used under GPLv2 or later. So that relicensing the codebase is not a blocker for this merge proposal to be integrated. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356147 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
This includes the external dependency pymediainfo now. This is available on all officially supported Linux distributions, so this should be okay. It also fixes a MIT license violation. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356147 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
Please note that I already removed the vlc changes. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356147 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
Removing VLC is moving against the direction agreed where VLC is going to be packaged into the windows installer to prevent issues caused by bad vlc installs. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356147 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
I always wondered why there is a setup.py that is not really functional... Making sure it can install openlp would be a good idea. It would greatly simplify packaging OpenLP (at least for archlinux) I don't see a downside to doing that. We only need to set it up once and then we only need to touch it when we modify dependencies or publish new versions. About removing the VLC code. IMO this is something we should really consider. I don't see why we should keep it in the repository. It's maintained somewhere else and we don't need to modify it. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356147 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
Hi Bastian, We're not trying to discourage you from contributing at all, and we'd never stop someone from contributing. We just want the contribution to line up better with the project's greater goals. If you have the motivation and the time to do it, that is awesome and we'd love you to do it. We just want to make sure that we are not negatively affecting existing platforms or plans. Having a discussion on our mailing list would make sure that all the issues that we are already aware of are discussed and we come up with a solution that works for most (or even all) situations. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356147 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
I see. The only change that would influence distributing via a Linux packaging system is the introduction of the vlc module as a new dependency. The other changes do not interfere if the Linux packages install python packages with their Python dist information which is the case for all the Linux distributions that I use. The vlc module available for Arch in the AUR. For Debian there is a Request For Packaging and on Fedora there is at least an old version for Python 2. I am happy to discuss changes. In my experience it is far better to have something to offer as a base for discussion in free software projects. If you just come with suggestions, the developers usually say: "Good idea! Maybe next year when someone has time..." The point of having a working setup.py is that you can install OpenLP on any platform that supports Python and Qt5. In the current state if you want to install e.g. on OpenBSD and did not know about scripts/check_dependencies.py you would have to go through a lot of files to get an idea of what dependencies are needed. And you could not be sure to be right so even if the program starts there could be a thing that you missed. E.g. I found the pyxdg when I started to scan OpenLP for license violations after I found the first one. It is not noted in check_dependencies at all. The same would be true for a developer with a Windows environment who wants to work on trunk. -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356147 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
Re: [Openlp-core] [Merge] lp:~bastian-germann/openlp/setup into lp:openlp
Review: Disapprove Hi Bastian, Thank you for contributing to OpenLP! It's great to see some of the issues I've seen in OpenLP finally addressed. We have not had the time to look into these issues, and we're happy to see someone looking into them. Unfortunately your changes are not compatible with distributing OpenLP via Linux distributions. While you have highlighted some very real problems in this merge proposal, your proposed fixes do not take into account the multiple ways OpenLP is distributed. While I am happy to make OpenLP installable via PyPI, it is not our primary way to distribute OpenLP. Most of our users are not developers or terribly computer literate for that matter, and certainly over 90% of them have never heard of or used PyPI. Please discuss your changes with the rest of the team on our mailing list. We have discussed some of the issues you have highlighted, and it would be better to engage the whole team and hear what we have already discussed and come up with a solution that works for everyone. Renaming openlp.py: https://lists.openlp.io/pipermail/openlp-dev/2017-March/000186.html Using Python 3.6: https://lists.openlp.io/pipermail/openlp-dev/2018-June/000402.html -- https://code.launchpad.net/~bastian-germann/openlp/setup/+merge/356147 Your team OpenLP Core is subscribed to branch lp:openlp. ___ Mailing list: https://launchpad.net/~openlp-core Post to : openlp-core@lists.launchpad.net Unsubscribe : https://launchpad.net/~openlp-core More help : https://help.launchpad.net/ListHelp
