Add package.el support #300
Conversation
|
Thanks a lot! JFYI, since this is a rather large change I won't have time to review it until Dec 30 or so. |
|
Not sure why the CI is failing, the log says |
Looks like it's failing the license check in examples/lib-2-pkg.el, probably best/easiest to add a license header to that file (even if the file is trivial). |
ty, obvious in hindsight |
darkfeline
force-pushed
the
pkg
branch
3 times, most recently
from
516ec5b
to
3deeb38
Compare
|
Is there an easy way to run CI locally? A naive "install bazel and |
That should generally work, you might need to install xmllint and texinfo locally though. |
|
For reference, additional deps I installed (to be updated until I get it fully working):
mm, I think I'm only getting failures for some coverage stuff now, not sure though. Something about missing coveragerc, which also looks to be hacked around in the code, so I'm inclined to throw it at the CI again. |
darkfeline
force-pushed
the
pkg
branch
3 times, most recently
from
eae171b
to
510a6ff
Compare
| @@ -408,6 +427,34 @@ To add a load path entry for the current package, specify `.` here.""", | |||
| doc = "List of `elisp_library` dependencies.", | |||
| providers = [EmacsLispInfo], | |||
| ), | |||
| "enable_package": attr.bool( | |||
| doc = """Enable generation of package.el package for this library. | |||
| This value is forced to False if testonly is True.""", | |||
There was a problem hiding this comment.
Why? Is there anything special about testonly that requires this?
There was a problem hiding this comment.
Libraries used only in tests aren't intended to be packaged and from the ones I've seen it's a little annoying to make them so. The point of the package support is to enable packaging, so it doesn't seem useful to enable it for testonly.
Edit: of course we could just manually set enable_package=False for all testonly targets, but since this would be needed for every single one AFAIK it seems easier to just do this.
| If there is no such package description file, then srcs must contain a file | ||
| `<name>.el` containing the appropriate package headers. | ||
|
|
||
| If there is only one file in srcs, then the default value is the file basename |
There was a problem hiding this comment.
hmm, not sure whether only using the basename is the right choice. We intentionally don't add the Bazel package directory to the load path by default so that filenames don't have to be globally unique; e.g. runfiles.el needs to be loaded as elisp/runfiles/runfiles, so it seems a bit weird to call the resulting package just runfiles. Maybe the default package name should also include the Bazel package name (i.e. elisp/runfiles/runfiles), or even the repo name?
There was a problem hiding this comment.
The package name can't have slashes. Note that this is just the default. In most cases, it is accurate to use the basename. In extraordinary cases explicitly supplying the package name is preferable IMO.
What do you mean by repo name?
There was a problem hiding this comment.
The package name can't have slashes.
Then maybe replace the slashes by dots, hyphens, or some other allowed character?
In most cases, it is accurate to use the basename.
I don't think so. Bazel organizes files in packages so that filenames don't need to be globally unique. It would also be confusing to have an Elisp package name that differs from the feature name in such major ways. For example, it would be hard to explain why the library elisp/proto/proto should have an Elisp package name that's just proto – presumably the library is in a non-root package to improve uniqueness of the library name.
In extraordinary cases explicitly supplying the package name is preferable IMO.
The default should still be unsurprising and somewhat conservative. If users do want a shorter/different Elisp package name, they can override the default.
What do you mean by repo name?
The name of the repository that the library is placed in. (However, that name also isn't part of the feature name, and the repo name can be somewhat unstable if e.g. the repo isn't a module published in the BCR.)
There was a problem hiding this comment.
Then maybe replace the slashes by dots, hyphens, or some other allowed character?
That does not work for the vast majority of cases. e.g. the magit package should indeed be magit and not blah-something-google-magit.
Bazel organizes files in packages so that filenames don't need to be globally unique.
Emacs packaging basically assumes one global flat namespace for all library files, so they do have to be unique unfortunately.
Specifically, package names must be unique among package names, and library file names must be unique among all library files across all packages/load-path dirs.
While it is possible to refer to and load libraries with slashes, this is not done in Emacs core even though Emacs distributes libraries in nested subdirs (all subdirs are added to load-path so libraries can be loaded by basename) or in any Emacs package manager, 1p or 3p that I know of. This is primarily used when Emacs is used as a batch interpreter, to explicitly load scripts via paths with slashes.
It would also be confusing to have an Elisp package name that differs from the feature name in such major ways
The feature name is expected to be the base name of the library file sans extension (note that a package can contain library files with names different from the package name, although conventionally a package should contain a library file with the same name as the package.).
I did a cursory check and all of our libraries also follow this convention (i.e., a package blah/something.el provides the feature something).
The default should still be unsurprising and somewhat conservative.
This default is unsurprising IMO. It works for almost all of our packages.
elisp/gen-autoloads.el
Outdated
| (create-lockfiles nil)) | ||
| (unwind-protect | ||
| (progn | ||
| (dolist (f srcs) |
There was a problem hiding this comment.
what happens if these files are in subdirectories? Shouldn't this preserve the directory structure?
There was a problem hiding this comment.
Packages are flat, so subdirs aren't supported (the packaging framework will only add the package dir to load-path). You can't have libraries with the same name in the load-path namespace anyway (one will override/shadow the other).
darkfeline
force-pushed
the
pkg
branch
9 times, most recently
from
a716d70
to
693059c
Compare
|
The Windows build is failing due to a random ASCII 15 character that seems to be getting added to Edit: it's 0o15 which is ASCII 13 which is carriage return. So the root cause is pretty obvious (and I feel stupid)... |
|
(FYI this is ready to land (or if you have any comments)) |
|
Friendly ping? |
|
Ping? |
Add package.el support so Emacs Lisp libraries can be installed as proper Emacs packages.