emacs-ess / ESS

Emacs Speaks Statistics: ESS
https://ess.r-project.org/
GNU General Public License v3.0
622 stars 162 forks source link

[review / help] Debian packaging using elpa framework #670

Closed eddelbuettel closed 6 years ago

eddelbuettel commented 6 years ago

I emailed about parts of this off-list with Alex; it had become time to turn Debian's ess package into elpa-ess. The reasons are mostly internal: we had a set of scripts to place and byte-compile elisp package source in place, we now do this via the elpa and package framework relying more on "upstream" which is generally the right thing. I am pretty ignorant of the details for Emacs but this is standard for Debian in as much as getting some infrastructure together for a set of packages. And Elisp is one such set of packages. Many now exist as elpa-* allowing fluid interplay between our repos and Emacs's.

Now, when doing this I needed to make two simple patches and I was wondering if you would want to look at these / fold them / make changes, or maybe these have to remain local. The first is more important as without it, the generated path of ..../package-version/ lacked the version:

--- ess-17.11.orig/lisp/ess-pkg.el
+++ ess-17.11/lisp/ess-pkg.el
@@ -1,2 +1,2 @@
-(define-package "ess" "0" "Emacs Speaks Statistics" '((julia-mode "0.3"))
+(define-package "ess" "17.11" "Emacs Speaks Statistics" nil
   :url "http://ess.r-project.org" :keywords nil)

The second may be due to me not (then) knowing about (package-initialize). Should this be needed?

--- ess-17.11.orig/lisp/ess-bugs-d.el
+++ ess-17.11/lisp/ess-bugs-d.el
@@ -23,6 +23,7 @@

 ;;; Code:

+(require 'ess-custom)
 (require 'ess-bugs-l)
 (require 'ess-utils)
 (require 'ess-inf)

I don't read all you post here (it is a very active repo) but I noticed a lot of autoload changes. Should this be better now?

jabranham commented 6 years ago

As for the first, ess-pkg.el won't exist for the upcoming 18.09 release. Does Debian rely on this file somehow?

We've already added ess-custom requirement to ess-bugs-d.el.

Thanks for the feedback!

eddelbuettel commented 6 years ago

The issue was not so much ess-pkg,el per se but the "0" for the version which we set there. AFAICT we'd need the current version set somewhere so that ${PACKAGE}-${VERSION} path can be built. That may be better now in (M)ELPA current; I only (re-)used the older 17.11 tarball.

jabranham commented 6 years ago

Does the contents of the file ./VERSION work? And/or the git tag?

eddelbuettel commented 6 years ago

Unsure. Depends on what the installer code does. We could also peek at other repos, they are all within this repo org: https://salsa.debian.org/emacsen-team/ (whereas ESS is at https://salsa.debian.org/edd/ess ).

jabranham commented 6 years ago

I don't know much about Debian packaging (I could talk more about Arch's PKGBUILDs or Guix's package-as-scheme-object approach), but this: https://salsa.debian.org/edd/ess/blob/master/debian/rules#L19 makes me think that Debian doesn't need ess-pkg.el at all. You can just declare the version in debian/changelog and bump that when needed. Have you tried adding ess-pkg.el to the list of ignored/excluded files?

(also, as an aside, since Debian is packaging things in their own gitlab instance now, can we remove the debian folder from this repo?)

eddelbuettel commented 6 years ago

can we remove the debian folder from this repo?

Yes, that thought had crossed my mind a few times too. Now in #671.

eddelbuettel commented 6 years ago

I'll try defining it in in debian/rules via debian/changelog. That is the preferred approach. Should I try a recent tarball / create a tarball to see if it works?

jabranham commented 6 years ago

Yes, go ahead and do that. make tarball (or make tarballs?) should give you one from a git clone. Let me know if you run into any issues.

vspinu commented 6 years ago

From what I see in the emacsen-team they just move original repo and add debian within. Most likely they use same strategy as melpa and that means using package-build which for us means we don't have to do anything. It should just work as it is. ess-pkg.el should not be needed. None of the archives on emacsen-team has it.

Questions: Can elpa-ess also be maintained as part of emacsen-team repo or it should be in edd/ess? Should the old ess debian go away eventually?

Should I try a recent tarball / create a tarball to see if it works?

Are you sure the tarball is still the way to go? I would say we make it as close to MELPA install as we can.

Please wait a sec. Our metadata as expected by package.el is sloppy. Let me fix it first.

vspinu commented 6 years ago

Done.

I have not much clue about Debian package system but from a quick comparison with other similar packages (cider for example) our rules seem to be quite a bit more complex.

We are no longer using changelog (for 5 years now) and debian changelog could therefore also be removed. I also think we should pick the version from one location ;; Version: field in ess.el where all package.el metadata lies.

eddelbuettel commented 6 years ago

they just move original repo and add debian within.

Yes. And which is exactly what we had here with debian/ inside upstream. [Edit: One important detail is, though, that upstream is always kept as-is in a branch pristine-tar from which each version can be extracted bit-by-bit identical to the upstream releases which is important. But git helps with the packaging workflow. ]

using package-build

We do now. As I wrote above, we used to have a different 'within Debian' framework which made sense for packaging of Emacs libraries within Debian in the pre elpa/melpa/... days. These days it makes sense to use Emacs packaging and so we do (within Debian) now.

Our metadata as expected by package.el is sloppy. Let me fix it first.

I think that was a core part of my question. The version did not transpire. If it does now, and if ess-custom is taken care then I guess we're done here. Thanks!

vspinu commented 6 years ago

Most likely it will go through. There are two version fields one Version which we and other packages set and Package-Version which MELPA (and I presume elpa-debian) auto-generate according to their needs.

Currently the Version is set to 18.09-dev which might be or not be what you need. Let's see what the build from current master generates.

eddelbuettel commented 6 years ago

That did not work quite as expected. If I say 'make tarballs' in master (at 72715b35) then I still get 17.11.

Should it not be 18.* something per a commit today?

eddelbuettel commented 6 years ago

And when I try that tarball, it ends in tears because a pkg file is apparently expected:

sed -i -e 's,(defvar ess-etc-directory nil,(defvar ess-etc-directory "/usr/share/ess/etc/",1'\
    -e 's,(while (and (listp ess-etc,(while (and (not ess-etc-directory) (listp ess-etc,1' \
        lisp/ess-site.el
dh_elpa
dh_elpa: missing ess-pkg.el; will try to generate it
dh_elpa: emacs -batch -Q -l package --eval (add-to-list 'package-directory-list "/usr/share/emacs/site-lisp/elpa") --eval (add-to-list 'package-directory-list "/usr/share/emacs/site-lisp/elpa-src") -f package-initialize -l dh-elpa.el -f dhelpa-batch-install-directory debian/elpa-ess//usr/share/emacs/site-lisp/elpa-src /build/ess-17.11/debian/.debhelper/elpa/ess /build/ess-17.11/debian/.debhelper/elpa 1536626978 returned exit code 255
dh_elpa: couldn't generate -pkg file; please write one
make: *** [debian/rules:66: install-stamp] Error 255
vspinu commented 6 years ago

tarball target picks the version from VERSION file. It's a bit of a mess. This is why I am saying that we should normalize all the tooling to work from one metatdata source ess.el file.

And when I try that tarball, it ends in tears because a pkg file is apparently expected:

The -pkg.el should be autogenerated and the failure happens when an auto-generation is attempted. Is there anything we could read about elpa-emacs infrastructure? There is nothing on https://salsa.debian.org/emacsen-team. Would you mind providing step by step instructions of how to mimic your error.? We pull https://salsa.debian.org/edd/ess and then what?

Let's take this opportunity and streamline the process. I see that magit which has similar nested file structure as ours does close to nothing in its rules. Can we also start from that and add strictly necessary ESS specific bits?

The current ESS/debian/rules has manual hacks (as that hardcoded sed in install-stamp) which either already fail or will fail at some point because the core elispers are oblivious of the debian infrastructure. We kept changing ess-site.el with no hesitation.

vspinu commented 6 years ago

There is a bit of info here and here. Let me see if I could figure that out.

eddelbuettel commented 6 years ago

Sorry replied first to PR thread and then here. I am not a programmer behind dh_elpa -- but there a dozens of such commands, generally implemented in Perl (ick) and prefixed dh_*. Ie there is dh_r as well a few people built and which we now use for CRAN packages (as R CMD foo ... is so bloody reliable to build upon).

The dh_elpa manual page says this at the bottom as I spotted last eve:

HINTS
   Specifying the package version
       If dh_elpa can't determine the package version by looking at *.el files (usually because
       upstream has failed to include the proper headers or *-pkg.el file), it will fallback to the
       DEB_UPSTREAM_VERSION and DEB_VERSION_UPSTREAM environment variables.  An easy way to set one of
       these based on your latest Debian changelog entry is just to prepend the following to your
       rules file:

           include /usr/share/dpkg/pkg-info.mk
           export DEB_VERSION_UPSTREAM

       Certain Debian upstream version strings cannot be translated into version strings Emacs will
       accept (see the docstring for the Emacs function `version-to-list' for details).  dh_elpa will
       error out if the version cannot be translated.  You should resort to patching in a Package-
       Version header or adding a *-pkg.el file.

That suggests we can get the version number from the debian/changelog (which, oddly, ess did for years too as I used to create the tarball I need from the repo this way; this may predate your current make tarballs rule). But dh_elpa may still look for the -pkg.el.

jabranham commented 6 years ago

The -pkg.el should be autogenerated

@vspinu not by us, right? Is this a Debian packaging feature?

eddelbuettel commented 6 years ago

No, I think by you guys. See my post here 17h ago showing our dh_elpa tool falling over.

As I understand, most if not all other elpa-* packages -- and there were 251 in Debian -- just call out to dh_elpa.

Of course if you don't or won't I can create a stub and have that used.

eddelbuettel commented 6 years ago

That said, if all we need is something like this:

(define-package "ess" "17.11" "Emacs Speaks Statistics" nil
  :url "http://ess.r-project.org" :keywords nil)

(from the previous build I released, 17.11-5) then I can of course trivially generate that from debian/rules as everything is either constant, or known (ie I will have the release in debian/changelog and also in VERSION.

So if you want me to do it at my end I can. Just let me know.

vspinu commented 6 years ago

That doc says by us but it's only in cases when we don't have metadata, which we do. dh_elpa has to know where .el lie. And for that you need elpa-ess.elpa with lisp/*.el or similar (see debian branch). Only a few elpa-debian packages have pkg.el, so it's clearly not needed.

As I already hinted earlier, unless a direction is to clean up current setup and model it after elpa-debian proper, I don't think we will end up with something long term useful here. Only hacks (like that pkg thing).

One more complication is that julia-mode.el is not on elpa-debian yet, so we will have to package that together (as we do now) but that's not a big deal.

eddelbuettel commented 6 years ago

Debian packaging has a lot of tiny details. Currently I just cp the *el in place from debian/rules; the dh_elpa framework then takes care of byte-compilation. But the whole dh_* framework also works in a declarative sense with files in debian/* -- and here, as I learned yesterday from reading man dh_elpa, I just declare the files (and that takes regexps too) in debian/package.elpa.

So from you I may really just need the two liner I quoted a message ago. Because if I can generate from your tarball, your Makefile can equally well put it into the tarball.

What I do not know as an Emacs Elisp noob is whether other users of M-x install-packages and Elpa would either benefit or even require it. If so, "your turn". If not, I'll do it locally. Ok?

vspinu commented 6 years ago

I just went ahead and tried it for myself, and ... sim sala bim, it worked. You can check with dpkg-buildpackage -b -rfakeroot -us -uc on the debian branch.

So now you have these versus current rules. If this is not an argument to move to a clean start I don't know what is.

The hack that you try to squeeze so hard from us is non-trivial. There are no tools except of some internal utilities in package-build (from melpa) which does that automatically. And I am damn sure we don't want to manually copy-paste or add sed-fu just for the sake of debian recipe, especially that we now know for sure that it works without.

jabranham commented 6 years ago

Maybe I'm missing something, but why is it a big deal to document ESS's package.el metadata in ess-pkg.el rather than ess.el? That's how we're supposed to do it anyway since ESS is a multifile package: (info "(elisp) Multi-file Packages")

vspinu commented 6 years ago

Because it's nicer and more convenient to keep it in a root file, and because almost no-one else is doing it that way, and because it's a redundant replication of metadata some of which should be in the root file anyhow (like authors, copyright, commentary), and because pkg.el is not designed to be read by humans (what is that second number in the list?) and probably because for a bunch of other reason ... and finally, because Dirk wants a quick hack instead of doing it the right way.

eddelbuettel commented 6 years ago

because Dirk wants a quick hack

WTF

jabranham commented 6 years ago

Because it's nicer and more convenient to keep it in a root file,

but now we have version in two places: VERSION and ess.el

no-one else is doing it that way

On my machine I find:

Since many (most?) packages I've got installed are single-file packages, that accounts for a hefty portion of multi-file packages.

redundant replication of metadata some of which should be in the root file anyhow (like authors, copyright, commentary)

The only required information is the package name, version, and dependencies. It's not supposed to have copyright info or commentary in it at all.

because pkg.el is not designed to be read by humans (what is that second number in the list?)

I'm assuming you're talking about package.el, but the second number is the version: C-h f define-package.

a quick hack instead of doing it the right way.

But using ess-pkg.el is the right way if we're working toward making ESS a package.el-friendly elisp package.

vspinu commented 6 years ago

that accounts for a hefty portion of multi-file packages.

Don't want to get into the count game with you, but yes, some packages do it indeed. Probably mostly older packages before package-build.

The only required information is the package name, version, and dependencies

And you prefer to split this data into a separate file instead of keeping it with all the rest? Just because someone wrote a package.el 10 years ago and couldn't figure out a parsing tool? There is nothing unhealthy about keeping all your metadata in one place.

MELPA does generate authors, maintainers (not copyright) and readme doc from the file. So we would need to to include that in the define-pacakge as well if we want to keep it consistent.

Melpa, pkg-build and dh_elpa parse headers just fine and at some point package.el will have to adopt it as a spec. It's annoying that if you write a single-file package you do it one way and if you have two files you do it completely differently.

And on a more general note, @jabranham you sometiems put too much weight on what is written in the elisp manual. Those are just people, writing and inventing stuff, a few of them probably with worse lisp skills than yours. So I would say we take it lighter and do what common sense dictates.

@eddelbuettel

WTF

A joke mate, a joke ...

vspinu commented 6 years ago

Regarding the dependencies, package-build aggregates those across different files. So you keep dependencies with tooling which they belong to (in our case ess-julia.el). Some people find that more natural, but -pkg.el wouldn't allow for it.

eddelbuettel commented 6 years ago

This issue probably never belonged here, was not discussed in the most helpful way, and is now addressed. I am not closing it, and I will refrain from opening issues here again.

jabranham commented 6 years ago

package-build is not in Emacs, so I don't think it's relevant to the discussion. It's a MELPA thing and ESS doesn't depend on MELPA.

I thought it was common sense to stick with the guidelines for package.el-friendly packages. Otherwise 3rd-party tools like the ones Debian wrote that expect elisp packages to be package.el friendly aren't happy, as we've seen here.

Like I said before, I'm not sure why it's so horrible to have a file with package.el metadata. It could easily replace VERSION right now. This would allow us to include unit tests for ESS packaging, distribution, installation and other things like autoloads, which would be nice as well. I'll drop it though.

eddelbuettel commented 6 years ago

As one of your downstream consumers, I would welcome that.

I do not have high hopes though.

vspinu commented 6 years ago

I'm not sure why it's so horrible to have a file with package.el

It's not horrible but it's uniformly inferior for many small reasons which add up to one big "no". Besides the quite a few such small aforementioned reasons here is yet another one. In our makefile we will have to treat that file specially for compilation, checkdoc and who knows what else. Plus it's simply annoying to see a ess-pkg.el in file completions all the time.

Debian wrote that expect elisp packages

How else can I emphasize that "debian does work" with our package and other hundreds of multi-file packages like ours? Any tool working on packages will have to recognize that "pkg.el" metadata failed its purpose and comply with that community decided to be the best practice.

It's really not a big deal. package.el knows how to parse one file, right? So it already knows how to parse our ess.el.

It could easily replace VERSION right now

Replacing VERSION was never an issue and is not relevant to this discussion. VERSION will be replaced from ess.el metadata in due time.

If -pkg.el will ever be truly needed (which I doubt) we could continue this discussion on duplicated and centralized metadata . For now it's really not clear that centralized is better than decentralized (think julia dependency or ESSR) and duplication is clearly bad. By complying to imperfect standards you make them stick. Want progress - make a stance.

In any case, this pkg.el topic is a secondary matter IMO. The far more important issue at stake is the shape of debian ESS distribution which I tried to focus the discussion on. For that plenty of input was provided, but it wasn't appreciated. Quite to the contrary. So I trust the topic is exhausted. Let's gently move on.

eddelbuettel commented 6 years ago

"debian does work" with our package

I am the relevant Debian maintainer here, and I assert that it does not, or does only if I postprocess.

Which is why I came here asking for a change. For some reason or other I do not really care about any more the discussion has not been as fruitful as others I had in 20+ years of packaging and interacting with upstream, so I am moving on. I suggest we lock this.

vspinu commented 6 years ago

I am the relevant Debian maintainer here, and I assert that it does not, or does only if I postprocess.

That's either because of your own conflicting setup or because you don't change our version before processing (just a wild guess). As I said in the very begging our current 18.09-dev is probably not what you need. You have to change it to valid debian release number. The real cause of the issue is up to you to figure out. The evidence that if you follow elpa-debian guidelines it does work as expected has been provided.

as fruitful as others I had in 20+ years of packaging

Well, if you keep being so dismissive about the non-trivial efforts to help you, then we are indeed done here.

eddelbuettel commented 6 years ago

I made a checkout on Monday evening and did make tarballs from master. It created 17.11. If you imagine something else then that is an issue at your end, as is your refusal to see Alex's argument for ess-pkg.el. I appreciate that you tried to help, the way you did that was simply a little too different from how it is commonly done, and we are arguing in circles. Peace out.

vspinu commented 6 years ago

I made a checkout on Monday evening and did make tarballs from master. It created 17.11.

The tarballs generate 17.11 because that'st he last released version. After the release it will be 18.09. I don't know what you expect here.

Your issues was that dh_elpa failed to pick the version, right? And that's orthogonal to the VERSION file in the root, or our make tarballs. The only obvious reason why I think dh_elpa could fail for you and not for me is because I change 18.09-dev to the fake released 18.09 in the ess.el header.

as is your refusal to see Alex's argument for ess-pkg.el.

I don't "refuse to see" arguments, I just count the arguments in favor of the clean ess.el metadata and they beat to the ground the alternative. It's all up there in the thread.

What you seem to misunderstand is that presence of ess-pkg.el is not the only nor the simplest solution to your problem. Our current metadata setup in ess.el is equivalent to ess-pkg.el for the purpose of dh_elpa. If it works with ess-pkg.el it should work with the current ess.el. Therefore there is no real need for -pkg.el. Does this make sense?