melpa / package-build

Tools for assembling a package archive
https://github.com/melpa/melpa
25 stars 34 forks source link

Various new features, mostly for use in recipes, and a bugfix #56

Closed tarsius closed 2 years ago

tarsius commented 2 years ago

They appear in this order:

  1. Add files from docs/ to default files-spec.
  2. Cosmetic changes to files-spec handling.
  3. Fix how last-modified revision is calculated, taking files into account that are not put in the tarball but which matter nonetheless. Support new (:inputs ...) form in the files-spec for that purpose.
  4. Add support for org-texinfo-export-to-texinfo and for specifying make targets and/or an arbitrary shell command in recipes. The unsafe code is restricted using bubblewrap, which obviously has to be installed on the server.
  5. Store revision description (e.g. v0.3.7-68-ga19faa1c) in archive.
  6. Give recipe code access to version information using PACKAGE_VERSION, PACKAGE_REVISION and PACKAGE_REVDESC envvars.
  7. When package-build.el is loaded and <package-build--melpa-base>/config.el exists, then load that too.

@purcell please have a look.

purcell commented 2 years ago

They appear in this order:

1. Add files from `docs/` to default files-spec.

Nice

2. Cosmetic changes to files-spec handling.

3. Fix how last-modified revision is calculated, taking files into account that are not put in the tarball but which matter nonetheless.  Support new `(:inputs ...)` form in the files-spec for that purpose.

What's the actual purpose of this one? I don't understand the use case, and feel like it could be avoided.

4. Add support for `org-texinfo-export-to-texinfo` 

Sounds like a good idea, but potentially dangerous if it allows any arbitrary code execution

and for specifying make targets and/or an arbitrary shell command in recipes. The unsafe code is restricted using bubblewrap, which obviously has to be installed on the server.

Again, use case? I've pushed back very strongly against supporting arbitrary build steps in recipes, at least for MELPA purposes. Even sandboxing such things isn't usually safe IMO.

5. Store revision description (e.g. `v0.3.7-68-ga19faa1c`) in archive.

Purpose?

6. Give recipe code access to version information using `PACKAGE_VERSION`, `PACKAGE_REVISION` and `PACKAGE_REVDESC` envvars.

7. When `package-build.el` is loaded and `<package-build--melpa-base>/config.el` exists, then load that too.

Same comment for these. I know I haven't been very involved recently, so I may be missing relevant discussions and context elsewhere.

tarsius commented 2 years ago

My overall goal is that I no longer want to check in any generated texi files into my repositories. I have wanted to do this for a very long time, but because it was clear that I would have to implement support for that in other software and that I can not be certain that those contributions would be accepted, I haven't done it until now. But bundling these generated files bothers me so much that eventually I just had to try.

3. Fix how last-modified revision is calculated, taking files
   into account that are not put in the tarball but which matter
   nonetheless.  Support new `(:inputs ...)` form in the
   files-spec for that purpose.

What's the actual purpose of this one? I don't understand the use case, and feel like it could be avoided.

For example, if a package splits its huge manual into multiple parts foo.texi and foo-second-part.texi but those files are expected to be exported to just one info file foo.info, then no new snapshot is created if the latest commit only modifies foo-second-part.texi.

This might be a bit theoretical. If my other changes are merged, then it becomes more likely that files that are not distributed themselves still affect the outcome.

Magit would need (:inputs "docs/.orgconfig" "docs/magit.org").

docs/.orgconfig is needed because it defines a macro and some strings that are used in docs/magit.org and docs/magit-section.org.

docs/magit.org should not be part of the tarball. Right now I build it locally using :make-targets ("texi" "info") and I have to make my improved ox-texinfo.el available until it is merged into Org and then Emacs 28.1 and that has been installed on the Melpa server. Currently these additional settings are necessary in config.el on my machine:

(setq package-build--sandbox-extra-ro-dirs
      '("/home/jonas/.emacs.d/lib/org/lisp/"))
(setenv "ORG_LOAD_PATH" "-L /home/jonas/.emacs.d/lib/org/lisp/")

For Magit it isn't possible yet to use just :info-manuals ("docs/magit.org") because that is intentionally kept simple and not as configurable as :make-targets and :shell-command.

Eventually I would like to use:

:files (... "docs/magit.org")
:info-manuals ("docs/magit.org")

The file has to be listed in info-manuals explicitly because otherwise it would not be possible to tell the difference between an Org file that should be included as-is and one that has to be exported to texi and then info.

4. Add support for `org-texinfo-export-to-texinfo`

Sounds like a good idea, but potentially dangerous if it allows any arbitrary code execution

There's a risk. An option would be to only allow trusted users to use these features in their recipes, but that would of course come with its issues too, such as "why am I not allowed to do it?".

and for specifying make targets and/or an arbitrary shell command in recipes. The unsafe code is restricted using bubblewrap, which obviously has to be installed on the server.

Again, use case?

I could live with just the less flexible approach where the manuals are specified using :info-manuals. That doesn't quite work for me yet, so I also looked into enabling more flexible options.

If feel that if we were to allow transcoding org to texi, then we might as well allow make targets and a shell command too.

Feature parity with [Non]GNU Elpa is also a motivation.

I've pushed back very strongly against supporting arbitrary build steps in recipes, at least for MELPA purposes. Even sandboxing such things isn't usually safe IMO.

Well that sounds like a firm no.

5. Store revision description (e.g. `v0.3.7-68-ga19faa1c`) in
   archive.

Purpose?

It has bothered me a great deal not being able to tell what version exactly a user is using when they report a bug. This will make it possible to determine that reliably.

By the way, if Emacs version comparison functions supported that, then I would strongly be in favor of using that as the actual version string. But I don't think there is any hope of that happening.

6. Give recipe code access to version information using
   `PACKAGE_VERSION`, `PACKAGE_REVISION` and `PACKAGE_REVDESC`
   envvars.

7. When `package-build.el` is loaded and
   `<package-build--melpa-base>/config.el` exists, then load
   that too.

Same comment for these.

The envvars can be used to bump version strings in manuals and such.

For my org-based manuals I use:

#+macro: version (eval (or (getenv "PACKAGE-REVDESC") (getenv "PACKAGE-VERSION") (ignore-errors (car (process-lines "git" "describe" "--exact"))) (ignore-errors (concat (car (process-lines "git" "describe" (if (getenv "AMEND") "HEAD~" "HEAD"))) "+1"))))

#+title: Magit User Manual
#+subtitle: for version {{{version}}}

This manual is for Magit version {{{version}}}.

config.el isn't intended for use on Melpa's servers but for other users of package-build.el that need to configure things a bit differently.

It's similar to the existing config.mk, which in my case contains:

LOAD_PATH = ~/.emacs.d/lib/package-build

As mentioned above my config.el currently contains:

(setq package-build--sandbox-extra-ro-dirs
      '("/home/jonas/.emacs.d/lib/org/lisp/"))
(setenv "ORG_LOAD_PATH" "-L /home/jonas/.emacs.d/lib/org/lisp/")

These files are not strictly necessary. I can make the necessary modifications locally and try to never check them in while working on other changes.

I know I haven't been very involved recently, so I may be missing relevant discussions and context elsewhere.

There haven't been any discussions that you have missed.

Cheers, Jonas

tarsius commented 2 years ago

Well Plan B is to use Github Actions for this and I just got the minimal viable version working!

tarsius commented 2 years ago

I have reordered the commits to put the harmless bits first.

What do you think about merging the first 11 or 12 commits?

milkypostman commented 2 years ago

Sorry for my inline comment reaction. I see @purcell kinda said the same things. I didn't realize the impact of these changes when I saw the emails come in.

Jonas, can you say more why the info-manuals thing doesn't work for you right now?

It's reasonable that info-manuals and files sections don't need to overlap. For example, magit should be able to do,

:files (...)
:info-manuals ("docs/magit.org")

where ... doesn't need to include docs/magit.org unless it actually wants docs/magit.org in the archive.

tarsius commented 2 years ago
1. Add files from `docs/` to default files-spec.

Nice

Okay, I have pushed that and once it is merged into melpa I will also update the readme of that.

tarsius commented 2 years ago

I am closing this for now. I might open new pull-requests for parts of this at a later time.