Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Debian packaging update #172

Closed
wants to merge 6 commits into from
Closed

Debian packaging update #172

wants to merge 6 commits into from

Conversation

leamas
Copy link
Contributor

@leamas leamas commented Apr 2, 2018

This updated PR updates the master branch, adding linux support to setup.py. On top of this the debian packaging is simplified and cleaned up to use the updated setup.py. Overall, this makes the package more of a "standard" package from a pypi, debian and other distro's point of view.

The fedora packaging (#170) is based on the master branch update as this PR.

Closes #166, #167 and #168. Also problems like #151 should be possible to walk around when the package could be installed using pypi (and using pypi's dependencies).

EDIT: I have not been able to check the MacOS build, and in particular the first commit 7727c04 might affect MacOS, so this need to be checked.

leamas and others added 6 commits April 2, 2018 17:54
Distutils/setup.py runs into potential problems when there is both a
module mkchromecast and a script mkchromecast. Resolve issue by moving
script to new location bin/.

Drop the .py suffix since it is a script and make it 755.

Add sys.path code to make it run both when installed and git-cloned.

The shebang is problematic. /usr/bin/python is wrong on most platforms,
since this is a python3 script. Updating to /usr/bin/python3 which
should be fine in many but not all cases -  the upstream python packages
just installs /usr/bin/python3.6 etc., not python3.

When the shebang fails, invocations like "python3.6 scripts/mkchromecast"
will always work (and should be documented somewhere).

Packaging toolchains typically patches the shebang as required, so this
is less of a problem in downstream packaging.
Add code to use setup.py also on Linux platforms. This is basically
to move installation code from the Debian packaging to the upstream
package. It makes the package a standard python package with correct
locations and will make maintenance as well as packaging on other distros
simpler.
Upstreamed patch from  debian packaging.
Since installation paths now are defined in upstream setup.py, clean
up rules and install files.
The tarball generation messes up the rules file. Also, having it
here make the usage less than obvious, requiring a fakeroot invocation
to build the tarball. A separate script make keeps rules tidy and is
more straight-forward to use.
@muammar
Copy link
Owner

muammar commented Apr 2, 2018

Thank you very much for these changes!. I was thinking that it would be a good idea to apply this PR to master instead of the debian branch. At the same time, I could move the debian/ directory to the master branch too. What do you think?.

@leamas
Copy link
Contributor Author

leamas commented Apr 2, 2018

Yes, this is my idea as well . The simple way would be to cherry-pick the three first commits 7727c04 ..f3b2beb into master and rebase the debian branch on new master before merging.

If you do this, we'll need to update the fedora packaging as well, basically drop the patches. I will be around for this.

You did check I didn't broke the MacOS build, rioght?

EDIT: short story: Don't move the debian directory to master. The debian packaging folks actively tries to persuade upstreams to not do this, and for good reasons. The separate debian branch is just fine, and there should really also be a separate pristine-tar branch. But that's another story...

@@ -33,7 +33,7 @@
whereas, installation from source needs users to go inside the cloned git
repository and execute:

python mkchromecast.py
mkchromecast
Copy link

Choose a reason for hiding this comment

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

Add a ./, because not all (none?) modern Unix-like operating systems assume that the current working directory is part of the PATH.

Copy link
Owner

Choose a reason for hiding this comment

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

The mkchromecast script is suppose to be in /usr/bin and when installing the debian package is placed there.

@@ -57,7 +57,7 @@
ALSA in your system.

Example:
python mkchromecast.py --encoder-backend ffmpeg --alsa-device hw:2,1
mkchromecast --encoder-backend ffmpeg --alsa-device hw:2,1
Copy link

Choose a reason for hiding this comment

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

Idem

@@ -77,10 +77,10 @@
Example:

ffmpeg:
python mkchromecast.py --encoder-backend ffmpeg -c ogg -b 128
mkchromecast --encoder-backend ffmpeg -c ogg -b 128
Copy link

Choose a reason for hiding this comment

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

The same and for all the following changes...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed... This was just an existing patch in the current debian packaging which I upstreamed as part of this. Actually the change goes python mkchromecast.py -> bin/mkchromecast I could rework the old patch or just drop it - it's not the important part here.

In other words: the documentation needs an overhaul (hey, this is user info in init.py...), and in this context this is probably the least problem. The more I think of it, tyhe proper solution for now would be to drop this patch.

Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

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

I never knew where was one supposed to add this type of documentation. If it is moved out of init.py, is another file needed to add all of this?

Copy link
Contributor Author

@leamas leamas Apr 3, 2018

Choose a reason for hiding this comment

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

This is really a discussion which belongs to #174. The short story is that there already is too many files, so adding yet another one is probably not the solution. In the long run, the user info should IMHO be centralized e. g. in the manpage , the website or README.md , with links from other locations.

Being the person I am, I would actually go for the manpage. Your mileage may vary, though.

EDIT: The old-fashioned Unix documentation, recognized also on MacOS is (according to no less than myself):

  • The --help info. Short and concise, should fit in a page or so.
  • The manpage: Detailed API and user info, expanding the --help summary.
  • The README files are really second class citizens here, adding distro-specific and platform quirks info.
  • The website... which should explain why this is such a great project, how to install it and where to find the usage info after installation.

@leamas
Copy link
Contributor Author

leamas commented Apr 3, 2018

OK, summing up the situation:'

  • The upstreamed documentation commit is plain wrong.
  • This PR applies both to master and debian branches, complicating things.
  • Muammar has expressed his interest to apply this PR to master.
  • The overall documentation situation is in a "could be improved" state.

Trying to cope with this I will make a new PR for master branch only, omitting the bad documentation commit. Once this is applied, this PR (as well as #170) should be rebased to the new master. Also, filing an overall documentation issue seems motivated.

EDIT: This is PR is basically dead, obsoleted by #173 and #174. Keeping it open for the sake of discussion

@muammar
Copy link
Owner

muammar commented Apr 3, 2018

I want to apply this to devel (except the debian/ changes to avoid conflicts with debian), and then make them available in master for the next release. I think these changes improve a lot this application.

@leamas
Copy link
Contributor Author

leamas commented Apr 3, 2018

Sounds reasonable. I suggest that you then apply #173 to devel instead of master. #173 is basically the master part of this PR. Once this is done, we could move on from there.

Do you want me to make a new PR identical to 173, besides targeting devel instead?

@muammar
Copy link
Owner

muammar commented Apr 3, 2018

@leamas I merged it to devel after solving a small conflict. Thanks for putting this together.

@leamas
Copy link
Contributor Author

leamas commented Apr 3, 2018

OK, great, I misunderstood the github messages.

Next stop should be rebasing #170 and #172 to devel, right?

How should we handle this? #170 is a simple rebase of a new branch, but debian/#172 is trickier unless you merge current devel into debian. Thoughts?

@rbrito
Copy link

rbrito commented Apr 5, 2018

About the layout of the repository, I found that making the master branch being "pure" and agnostic about distributions (using only vanilla Python PyPI packaging with setup.py) is best...

Then, you put the packaging branch in a branch like debian and, then, you instruct git-buildpackage to grab the sources from there (after everything being tagged properly).

@leamas
Copy link
Contributor Author

leamas commented Apr 5, 2018

Agreed. It's also exactly what we have now: devel/master under way to get the pypi packaging in shape, and separate branches for debian and fedora packaging.

@leamas
Copy link
Contributor Author

leamas commented Apr 5, 2018

Closing this, the debian part needs to be rebased to current devel.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants