search

raelgc / scudcloud

ScudCloud - Slack for Linux
https://launchpad.net/~rael-gc/+archive/ubuntu/scudcloud
MIT License
1.22k stars 99 forks source link

Debian package format should be native #512

Open onitake opened 7 years ago

onitake commented 7 years ago

ScudCloud Version

ScudCloud 1.38

Distro and Desktop info

Expected behavior

Scudcloud includes Debian package building scripts in its source archive. Those are updated as new package versions are released, so the package should be considered 'native', and it should not include a Debian release version (such as -1).

If patch releases are desired, they should be named as part of the release version, i.e.: Release 1.38 -> Debian version 1.38.0 Release 1.38 Patch 1 -> Debian version 1.38.1 Or similar.

Actual behavior

The package source format is 'quilt' and the package version includes a Debian release.

Issues

When building the package using standard Debian tools, debuild in particular, they will throw errors because an upstream source package is missing. In case of a native package, this is not necessary, because the sources contain both the package building scripts and the source code itself.

Recommendations

To fix this, change debian/source/format to contain: 3.0 (native)

Furthermore, please drop the Debian patch version from future changelog entries. The entry scudcloud (1.38-1) yakkety; urgency=medium will then become scudcloud (1.38.0) yakkety; urgency=medium

raelgc commented 7 years ago

Hi @onitake and thanks for raising this.

The problem is that I'm generating this package primary for Ubuntu.

I would love if someone can build them in Debian, and then I just convert to Ubuntu.

If I'm misunderstanding the matter, please, correct me and point the correct direction.

onitake commented 7 years ago

This is actually not Debian-specific - the same rules can be applied to all distributions that use Debian packaging tools. Debian versions were specifically introduced for software that does not maintain their own dpkg scripts, so additional patches can be added easily on top of the upstream source code. That's what quilt is used for, by the way. If the sources already include a maintained debian/ tree, it's best to set format to native and ditch the Debian version altogether.

Aside from this minor issue, scudcloud builds and runs perfectly well on Debian. On stable, it may be necessary to use the qt4 version - I only tested the qt5 branch on testing so far.

Note that I am not a full-fledged Debian developer yet, and I still need a mentor to push packages, but I can do a full review and submit the package on your behalf, if you like.

raelgc commented 7 years ago

@onitake Thank you for the explanation! Now I got it.

Well, I like the idea of getting a Debian developer working on a package for Scudcloud on Debian. And if additionally this helps you to get a mentor, so best of both worlds :)

raelgc commented 7 years ago

@onitake Any progress on get a Debian mentor? :)

onitake commented 7 years ago

Finding a mentor who will take care of uploading should be less of a problem, and once a package has been accepted, updates will be accepted faster. The bulk of the work lies in getting a package release-ready.

Aside from the mentioned versioning conflict, Lintian reports a number of other problems:

$ lintian -I
E: scudcloud source: source-is-missing scudcloud/resources/leftpane.js line length is 1635 characters (>512)
E: scudcloud source: source-is-missing scudcloud/resources/scudcloud.js line length is 3264 characters (>512)
W: scudcloud source: out-of-date-standards-version 3.9.7 (current is 3.9.8)
W: scudcloud: latest-debian-changelog-entry-changed-to-native
E: scudcloud: description-starts-with-package-name
E: scudcloud: description-synopsis-is-duplicated
W: scudcloud: extra-license-file usr/share/doc/scudcloud/LICENSE
W: scudcloud: binary-without-manpage usr/bin/scudcloud

The latest-debian-changelog-entry-changed-to-native warning can be ignored/overridden (obviously), but the rest should be addressed if possible. In detail:

raelgc commented 7 years ago

JavaScript files should be included in their entirety instead of minified versions - I think the preferred way is adding beautified JS files and running them through uglifyjs (or similar) as a build step. How are you maintaining the JS sources of ScudCloud?

This warning is a false negative. They're in the sources folder and I already tried to make the package build recognize them as source for the 2 files, but appears to be a bug in the package builder. An alternative, like you described, would be use an inline uglyfier/minifier during the build process. Any suggestion on this? Can you create a PR?

Other errors/warnings, like you said, are super easy to fix.

onitake commented 7 years ago

Well, the heuristics employed by Lintian are not very smart, and the Debian developers are very picky about most of the checks. On the other hand, the pickiness helps finding details that are easily missed or seem irrelevant but could bite you later.

I'll see how the JS error can be solved. And if that's not possible, an override with an explanation should do the trick.

onitake commented 7 years ago

All right.

I found out how to add custom build steps to setuptools. Here's a patch that deals with the JS files.

Please take a look at the PR description, there may be one possible issue that needs to be addressed.

raelgc commented 7 years ago

@onitake Thanks for the PR.

But, a lot of users run the app from the dev tree.

I can see you didn't remove the minified files from the repo. If I correctly understood, this will still enable users to clone the repo and run from the dev tree, right?

onitake commented 7 years ago

I did remove the files, because otherwise they would be picked up by Lintian again.

How do you run scudcloud from the dev tree, normally?

I suppose a good solution could be moving the JavaScript files from sources/ to scudcloud/resources/, and do all processing during installation. This could simply be accomplished by excluding *.js from package_data - MinifyJsBuildCommand installs them to the correct staging directory already.

What do you think?

onitake commented 7 years ago

Speaking of package_data, I just discovered this.

I suppose it's useless anyway, just put everything except leftpane.js and scudcloud.js in MANIFEST.in, and let setuptools and MinifyJsBuildCommand take care of the rest? Let me try that.

onitake commented 7 years ago

Ok. That seems to do the trick.

Running from the source tree (with python3 -m scudcloud) works. Building and running the Debian package works. The source dist contains all relevant files - including the un-minified JavaScript sources.

Can you test the PR again and see if it works for you too?

raelgc commented 7 years ago

Checking the PR quickly, it'll generate the minified files during the install, right? (.deb install or python setup).

Users running from dev tree will use the uniminified versions?

I'm fine with this solution, just double checking to see if I correctly understood.

onitake commented 7 years ago

Yes exactly, that is the idea.

raelgc commented 7 years ago

Thank you for the awesome improvement, @onitake! :+1:

raelgc commented 7 years ago

Something we still can do to advance on this subject?

onitake commented 7 years ago

First and foremost, implement the version change. :)

Then, we need to address the Lintian errors. After that, I will build the package and send it to mentors.debian.org. Then I need to find a sponsor that will accept it.

raelgc commented 7 years ago

@onitake Using .0 instead of -1, right?

onitake commented 7 years ago

You also need the source format to native, as I wrote in the Recommendations above.