Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Add package.el support #300

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add package.el support #300

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions elisp/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,42 @@ bootstrap(
],
)

bootstrap(
name = "gen-pkg-el",
src = "gen-pkg-el.el",
out = "gen-pkg-el.elc",
visibility = [
# FIXME: Make private once
# https://github.com/bazelbuild/proposals/blob/main/designs/2019-10-15-tool-visibility.md
# is implemented in all supported Bazel versions.
"//visibility:public",
],
)

bootstrap(
name = "gen-metadata",
src = "gen-metadata.el",
out = "gen-metadata.elc",
visibility = [
# FIXME: Make private once
# https://github.com/bazelbuild/proposals/blob/main/designs/2019-10-15-tool-visibility.md
# is implemented in all supported Bazel versions.
"//visibility:public",
],
)

bootstrap(
name = "gen-autoloads",
src = "gen-autoloads.el",
out = "gen-autoloads.elc",
visibility = [
# FIXME: Make private once
# https://github.com/bazelbuild/proposals/blob/main/designs/2019-10-15-tool-visibility.md
# is implemented in all supported Bazel versions.
"//visibility:public",
],
)

build_test(
name = "compile_test",
targets = [":compile"],
Expand Down
213 changes: 212 additions & 1 deletion elisp/defs.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ additions for this library and all its transitive dependencies.
The `depset` uses preorder traversal: entries for libraries closer to the root
of the dependency graph come first. The `depset` elements are structures as
described in the provider documentation.""",
"package_file": """A `File` object for the -pkg.el file.
None if `enable_package` is False.""",
"metadata_file": """A `File` object for the metadata file.
None if `enable_package` is False.""",
"autoloads_file": """A `File` object for the autoloads file.
None if `enable_package` is False.""",
},
)

Expand All @@ -79,9 +85,19 @@ def _elisp_library_impl(ctx):
tags = ctx.attr.tags,
fatal_warnings = ctx.attr.fatal_warnings,
)
extra_out = []
package_file = None
metadata_file = None
autoloads_file = None
if ctx.attr.enable_package and not ctx.attr.testonly:
pkg = _build_package(ctx, ctx.files.srcs, ctx.files.data)
extra_out += [pkg.package_file, pkg.metadata_file, pkg.autoloads_file]
package_file = pkg.package_file
metadata_file = pkg.metadata_file
autoloads_file = pkg.autoloads_file
return [
DefaultInfo(
files = depset(direct = result.outs),
files = depset(direct = result.outs + extra_out),
runfiles = result.runfiles,
),
coverage_common.instrumented_files_info(
Expand All @@ -97,6 +113,9 @@ def _elisp_library_impl(ctx):
transitive_source_files = result.transitive_srcs,
transitive_compiled_files = result.transitive_outs,
transitive_load_path = result.transitive_load_path,
package_file = package_file,
metadata_file = metadata_file,
autoloads_file = autoloads_file,
),
]

Expand Down Expand Up @@ -344,6 +363,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.""",
Copy link
Owner

Choose a reason for hiding this comment

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

Why? Is there anything special about testonly that requires this?

Copy link
Contributor Author

@darkfeline darkfeline Jan 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Is it then possible to test package builds at all? For example, I'd like the analysis test in tests/private/defs.bzl to contain a test for the new functionality.

Maybe it would be better to have a tri-state Boolean (e.g. "always", "never", "auto").

default = True,
),
"emacs_package_name": attr.string(
doc = """The name used for the package.el package.
This attribute is ignored if enable_package is False.
Otherwise, srcs should contain a package description file `<name>-pkg.el`.
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
Copy link
Owner

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Owner

Choose a reason for hiding this comment

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

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.)

Copy link
Contributor Author

@darkfeline darkfeline Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Yes, in such a case the BUILD file should specify the appropriate package name.

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.

Emacs core does use library/feature names with slashes, namely in the "cedet" and "term" subdirectories. Granted, it's not very common, but does happen.

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.).

While this is a common convention, it's not universal nor required.

The default should still be unsurprising and somewhat conservative.

This default is unsurprising IMO. It works for almost all of our packages.

It's not so important how the distribution for each approach is because the two choices aren't symmetric.

  1. If the basename without directory is chosen as default for the package name, it's likely that unintentionally non-unique package names are created. That's bad because it will result in nasty clashes which might be noticed only when it's too late, e.g. when packages from multiple repositories are merged.
  2. On the other hand, if the repository-relative filename is chosen as default, such clashes are less likely because filenames across Bazel repositories are generally assumed to be unique already (even ignoring the repository name). An unintentionally wrong default will then only result in an overly long package name, which is annoying but not harmful.

Given that defaults should be safe, (2) would be the better choice.

There are approaches to improve the default, at the expense of a bit more coding complexity. Specifically, the rule author has already stated their intention regarding uniqueness with the load_path attribute: In the common case of load_path = ["."], the package-relative filename is already expected to be globally unique, and the package build code can make use of that expectation. Specifically, I think the following algorithm should give good results in most cases:

  1. Normalize the filename so that it's repository-relative (i.e. prepend the Bazel package name to the package-relative filename).
  2. Normalize all load path entries so that they are repository-relative as well (see the logic in _compile in elisp/defs.bzl).
  3. Relativize the normalized filename against the normalized load path, i.e. strip each load path element in turn from the normalized filename. Fail if that doesn't work, i.e. if the filename isn't inside the load path directory.
  4. Take the shortest of the relative filenames. If there's no unique shortest relative filename, fail.
  5. Strip the extension to turn the filename into a feature name.
  6. Replace slashes in the feature name to obtain a valid Emacs package name.

For example, if a package foo/bar contains a file baz/qux.el with

elisp_library(name = "qux", srcs = ["baz/qux.el"], load_path = ["/foo", "."])

this will result in

  1. foo/bar/baz/qux.el
  2. ["foo", "foo/bar"]
  3. ["bar/baz/qux.el", "baz/qux.el"]
  4. baz/qux.el
  5. baz/qux
  6. baz-qux

Such a file would need to provide at least one of the features baz/qux and bar/baz/qux for require to work, so the generated Emacs package name would match the shortest possible feature name.

with the .el suffix removed. Otherwise, the default is the target label name,
with underscores replaced with dashes.""",
),
"_gen_pkg_el": attr.label(
default = "//elisp:gen-pkg-el.elc",
allow_single_file = [".elc"],
),
"_gen_metadata": attr.label(
default = "//elisp:gen-metadata.elc",
allow_single_file = [".elc"],
),
"_gen_autoloads": attr.label(
default = "//elisp:gen-autoloads.elc",
allow_single_file = [".elc"],
),
},
doc = """Byte-compiles Emacs Lisp source files and makes the compiled output
available to dependencies. All sources are byte-compiled.
Expand Down Expand Up @@ -1216,6 +1263,170 @@ def _resolve_load_path(ctx, dir):
),
)

def _get_emacs_package_name(ctx):
"""Returns the package name to use for `elisp_library' rules."""
if ctx.attr.emacs_package_name:
return ctx.attr.emacs_package_name
if len(ctx.files.srcs) != 1:
return ctx.label.name.replace("_", "-")
basename = ctx.files.srcs[0].basename
if not basename.endswith(".el"):
fail("Suspicious single file when guessing package_name for target", ctx.label)
if basename.endswith("-pkg.el"):
fail("Suspicious package_name derived from single source file for target", ctx.label)
return basename[:-len(".el")]

def _build_package(ctx, srcs, data):
"""Build package files.

Args:
ctx (ctx): rule context
srcs (list of Files): Emacs Lisp sources files
data (list of Files): data files

Returns:
A structure with the following fields:
package_file: the File object for the -pkg.el file
metadata_file: the File object containing the package metadata
autoloads_file: the File object for the autoloads file
"""
package_name = _get_emacs_package_name(ctx)

pkg_file = None

# Try to find an existing -pkg.el file
expected = package_name + "-pkg.el"
for file in srcs + data:
if file.basename == expected:
pkg_file = file
break

# Generate a -pkg.el file
if pkg_file == None:
expected = package_name + ".el"
for file in srcs + data:
if file.basename == expected:
pkg_file = _generate_pkg_el(ctx, file)
break
if pkg_file == None:
fail("No package metadata found for target", ctx.label)

# Try to find an existing autoloads file
autoloads_file = None
expected = package_name + "-autoloads.el"
for file in srcs + data:
if file.basename == expected:
autoloads_file = file
break
if autoloads_file == None:
autoloads_file = _generate_autoloads(ctx, package_name, srcs)
metadata_file = _generate_metadata(ctx, pkg_file)
return struct(
package_file = pkg_file,
metadata_file = metadata_file,
autoloads_file = autoloads_file,
)

def _generate_pkg_el(ctx, src):
"""Generate -pkg.el file.

Args:
ctx (ctx): rule context
src (File): Emacs Lisp source file to parse for package metadata

Returns:
the File object for the -pkg.el file
"""
package_name = src.basename.rsplit(".")[0]
out = ctx.actions.declare_file(paths.join(
_OUTPUT_DIR,
ctx.attr.name,
"{}-pkg.el".format(package_name),
))
inputs = depset(direct = [src, ctx.file._gen_pkg_el])
run_emacs(
ctx = ctx,
arguments = [
"--load=" + ctx.file._gen_pkg_el.path,
"--funcall=elisp/gen-pkg-el-and-exit",
src.path,
out.path,
],
inputs = inputs,
outputs = [out],
tags = ctx.attr.tags,
mnemonic = "GenPkgEl",
progress_message = "Generating -pkg.el {}".format(out.short_path),
manifest_basename = out.basename,
manifest_sibling = out,
)
return out

def _generate_metadata(ctx, package_file):
"""Generate metadata file.

Args:
ctx (ctx): rule context
package_file (File): the File object for the -pkg.el file

Returns:
The File object for the metadata file
"""
if not package_file.basename.endswith("-pkg.el"):
fail("Unexpected package_file", package_file)
package_name = package_file.basename[:-len("-pkg.el")]
out = ctx.actions.declare_file(paths.join(_OUTPUT_DIR, ctx.attr.name, "{}.json".format(package_name)))
inputs = depset(direct = [package_file, ctx.file._gen_metadata])
run_emacs(
ctx = ctx,
arguments = [
"--load=" + ctx.file._gen_metadata.path,
"--funcall=elisp/gen-metadata-and-exit",
package_file.path,
out.path,
],
inputs = inputs,
outputs = [out],
tags = ctx.attr.tags,
mnemonic = "GenMetadata",
progress_message = "Generating metadata {}".format(out.short_path),
manifest_basename = out.basename,
manifest_sibling = out,
)
return out

def _generate_autoloads(ctx, package_name, srcs):
"""Generate autoloads file.

Args:
ctx (ctx): rule context
package_name (string): name of package
srcs (list of Files): Emacs Lisp source files for which to generate autoloads

Returns:
The generated File.
"""
out = ctx.actions.declare_file(paths.join(_OUTPUT_DIR, ctx.attr.name, "{}-autoloads.el".format(package_name)))
inputs = depset(direct = srcs + [ctx.file._gen_autoloads])
run_emacs(
ctx = ctx,
arguments = [
"--load=" + ctx.file._gen_autoloads.path,
"--funcall=elisp/gen-autoloads-and-exit",
out.path,
package_name,
ctx.actions.args().add_all(srcs),
],
inputs = inputs,
outputs = [out],
tags = ctx.attr.tags,
mnemonic = "GenAutoloads",
progress_message = "Generating autoloads {}".format(out.short_path),
manifest_basename = out.basename,
manifest_sibling = out,
)
return out

# Directory relative to the current package where to store compiled files. This
# is equivalent to _objs for C++ rules. See
# https://bazel.build/remote/output-directories#layout-diagram.
Expand Down
63 changes: 63 additions & 0 deletions elisp/gen-autoloads.el
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
;;; gen-autoloads.el --- generate autoloads file -*- lexical-binding: t; -*-

;; Copyright 2021 Google LLC
;;
;; Licensed under the Apache License, Version 2.0 (the "License");
;; you may not use this file except in compliance with the License.
;; You may obtain a copy of the License at
;;
;; https://www.apache.org/licenses/LICENSE-2.0
;;
;; Unless required by applicable law or agreed to in writing, software
;; distributed under the License is distributed on an "AS IS" BASIS,
;; WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
;; See the License for the specific language governing permissions and
;; limitations under the License.

;;; Commentary:

;; Generate an autoloads file.
;;
;; Usage:
;;
;; emacs --quick --batch --load=gen-autoloads.el \
;; --funcall=elisp/gen-autoloads-and-exit DEST PKGNAME SOURCE...
;;
;; Generates an autoloads file DEST for the SOURCE Emacs Lisp files.
;;
;; Exits with a zero status only if successful.

;;; Code:

(require 'cl-lib)
(require 'nadvice)
(require 'package)

(defun elisp/gen-autoloads-and-exit ()
"Generate an autoloads file and exit Emacs.
See the file commentary for details."
(unless noninteractive
(error "This function works only in batch mode"))

(add-to-list 'ignored-local-variables 'generated-autoload-file)

(pcase command-line-args-left
(`(,out ,pkgname . ,srcs)
(let* ((workdir (file-name-as-directory (make-temp-file "workdir" :dir)))
;; Leaving these enabled leads to undefined behavior and doesn’t make
;; sense in batch mode.
(attempt-stack-overflow-recovery nil)
(attempt-orderly-shutdown-on-fatal-signal nil)
(create-lockfiles nil))
(dolist (f srcs)
(copy-file f workdir))
(package-generate-autoloads pkgname workdir)
(copy-file
(expand-file-name (concat workdir (format "%s-autoloads.el" pkgname))
workdir)
out t)
(kill-emacs 0)))
(_ (error "Usage: emacs elisp/gen-autoloads.el DEST PKGNAME SOURCE..."))))

(provide 'elisp/gen-autoloads)
;;; gen-autoloads.el ends here
Loading