Add straight--run-build-commands

[progfolio]

Command takes a recipe and executes it's :build commands.
See: #72, #389

[progfolio]

@raxod502:

Have some spare time to look into #72.
This pull is rough, but I figured I'd submit now, so we can get on the same page about the overall design
and you can point me in the right direction if I'm not leveraging the existing code base properly.
I'm sure there are more cases to consider, too, but this is a rough draft:

I've added a function, straight--run-build-commands, which accepts a recipe.
The recipe's :build keyword is an alist of the form:

((system-type . ((command)...)))

For example, I'm able to now build org-plus-contrib with straight-fix-org set to nil and the following recipe:

(straight-use-package
 '(org-plus-contrib
   :type git
   :repo "https://code.orgmode.org/bzg/org-mode.git"
   :local-repo "org"
   :build ((gnu/linux . (("make" "oldorg" "EMACS=/usr/bin/emacs"))))
   :files (:defaults "contrib/lisp/*.el")))

However, the generated file (./lisp/org-version) is not properly symlinked yet.

In its current state, there is no way to specify commands without specifying a system name.
I plan on addressing this, but have a couple questions:

Do we only want to support shell commands, or should we have a way to support arbitrary Elisp as well?
Supporting Elisp commands would make other use cases possible (static analysis of code),
but would complicate the implementation and may be beyond what you have in mind.

What is the expected behavior in cases where a recipe has no generic build commands and the user's system
does not match any of the system types? Try the first available command? Error?

There's also the question of how we want to express the "generic" build commands in the :build alist.

;;system specific
:build ((gnu/linux . (("make" "oldorg" "EMACS=/usr/bin/emacs"))))

;;generic
:build (("make" "oldorg" "EMACS=/usr/bin/emacs"))

;;system specific with generic fallback?
:build ((berkely-unix . (("gmake" "oldorg" "EMACS=/usr/bin/emacs")))
        (("make" "oldorg" "EMACS=/usr/bin/emacs")))

straight.el has been a valuable tool for me. Looking forward to contributing something back.

Thanks, Nicholas Vollmer.

[progfolio]

Another concern with el-get packages:
https://github.com/dimitri/el-get/blob/master/recipes/mu4e.rcp#L6-L12

It looks like el-get supports a :post-init keyword. Some packages, mu4e, for example rely on these commands to properly set the executable path. Personally, I think it would be simpler to just set the path in one's configuration for mu4e rather than the recipe itself.

[raxod502]

However, the generated file (./lisp/org-version) is not properly symlinked yet.

Is this just because it needs to be added to :files? If so, that's a fine solution, as straight.el already ships hardcoded recipes for Org.

Do we only want to support shell commands, or should we have a way to support arbitrary Elisp as well?

We should support whatever el-get supports. I do not think we should support anything further than that, though. Package authors should simply not use build commands, in my opinion, and we should not encourage that. The idea of #72 is just to steal work from el-get, and to do that the recipe syntax must be supported.

What is the expected behavior in cases where a recipe has no generic build commands and the user's system does not match any of the system types?

Probably whatever el-get does.

There's also the question of how we want to express the "generic" build commands in the :build alist.

Again, can we just do the same thing as el-get? The more similar the recipes are, the easier it will be to write the adapter that turns el-get recipes into straight.el recipes.

Personally, I think it would be simpler to just set the path in one's configuration for mu4e rather than the recipe itself.

You are assuredly correct. Nevertheless, it will be easy to support, so we should support it. We can just cl-letf emulated implementations of the relevant el-get functions, somewhat like we bind variables while evaluating package autoloads.

---

Let me know if I missed anything. Thanks for looking at this!

[progfolio]

I'm still unclear about how much of el-get's recipe format you intend to support. Here's a list of the keywords el-get recipes support:

:autoloads
:branch
:checkout
:compile
:depends
:features
:info
:lazy
:load
:load-path
:minimum-emacs-version
:module
:name
:pkgname
:post-init
:prepare
:shallow
:type
- builtin
- bzr
- elpa
- emacswiki
- ftp
- git
- github
- hg
- http
- http-zip
- svn
:url
:website

Some of these keywords offer functionality that overlaps with straight/use-package. Some do not.
Implementing all of these seems like it's beyond the scope of this pull.

Grepping over el-get's recipes, 95 out of 1782 (roughly 0.053%) utilize the :build keyword.
I see that straight--compile-package-texinfo already covers the case of recipes which use :build purely for compiling info docs.
Ignoring those, the recipe count is roughly on the order of 60.
Perhaps it would be easier to develop straight's notion of a :build keyword first and hand translate the subset of el-get's recipes that require building? This would solve the immediate problems of building packages like Org, mu4e, etc. without resorting to hacks (e.g. straight-fix-org).
Support for el-get recipes could still be built on top of this functionality in a separate pull if/when desired.

Thoughts?

[raxod502]

Implementing all of these seems like it's beyond the scope of this pull.

Absolutely. I don't want to suggest that you need to add el-get compatibility. Just a simple :build recipe support would be quite fine. All I mean is that when adding a specific feature that's already provided by el-get, why not implement it using a similar format, to make it easier to add compatibility later?

Please don't feel like you need to do any work that is not needed for the feature you want to implement now.

[progfolio]

FWIW, I've been using what we have here with manually specified build recipes successfully for Org and mu4e. I don't personally use any other packages that require build steps at the moment.

Some thoughts after using this for a couple weeks:

First, straight already has the notion of "buildling" a package (byte-compiling/autoloads/symlinking) so I wonder if it would be useful to come up with terminology to distinguish that "build" from the :build system commands that are being issued from recipes. It will help clarify things in the documentation and when reporting issues. I haven't put too much thought into this and I think it will be very easy to implement once the terminology is sorted (e.g. just emit a different message when "building" vs ":building").

Secondly, when developing a package, one may not want to run the :build step after every init. For example, when working on an Org bug, I don't want to :build (re-running make, etc) unless I have to. Same goes for mu4e. The :build is usually overkill as straight already takes care of byte-compilation/autoloads and make is just being used to generate some dynamic data or info on install.
I've been working around this by manually setting :build to nil when developing a package that I know doesn't need it's :build run with what I'm changing. I've yet to think of a better solution.

Thirdly, and most importantly, are security concerns. Do we want to encourage users to trust recipes that may run arbitrary system commands? Perhaps we could store a hash of the :build instructions somewhere and have a user-configurable policy for what to do when that hash changes. Users could choose to ignore the change and run the :build (by prompt or default), prevent building altogether, or just be notified of the change (it would be nice if we could show the user a diff of the build recipe, as this would make it easier for those who are not developers to at least be aware of what changed). This would involve more machinery around the :build step, but it would only impact the few recipes that utilize that keyword. This should be configurable globally and able to be overridden on a per-recipe basis. Worth considering, IMO.

Let me know what you think and what we should work on next.

[raxod502]

I wonder if it would be useful to come up with terminology to distinguish that "build"

You are very keen-eyed. Terminology is indeed a problem with straight.el, what with "packages" that can mean packages or libraries or features, and "use-package" that can refer to use-package.el or straight-use-package, and some other points of common confusion.

Perhaps :compile or :system or :shell or something of that nature would be more explanatory. I think :build is just fine as well, and you are right that the message can be changed regardless.

when developing a package, one may not want to run the :build step after every init

What if, instead of running the build commands in the build directory, we run them in the source directory, before symlinking? Then, anything related to make will automatically be fast because of the dependency tracking. I think this is actually how el-get itself works (i.e., it runs commands directly in the repository).

Perhaps, in the future when support for el-get is added to straight.el, we can add the option for .elc files to be linked into the build directory from the source repository, and then byte-compilation for those files could be skipped. This would be the best possible performance, I think.

most importantly, are security concerns

Unfortunately, I think you may be a bit optimistic about our prospects here. It's impossible to make package updates secure without locking down the entire package. This is because almost every step of the build process allows arbitrary command execution. In particular, byte-compilation and autoload evaluation both give an attacker just as much power as the :build specification in a recipe.

I have not attempted to address these problems, because they exist in every Emacs package manager and I don't see a good way of avoiding them without introducing a dizzying amount of overhead to everyday usage. There has been occasional criticism of straight.el for this reason, which is well-deserved (although it's not really a straight.el problem, but an ecosystem problem), but I think improving the situation will require a massive effort or some extreme cleverness.

Let me know what you think and what we should work on next

I think it would be prudent to make build commands be run in the source repository, if you agree about that idea, since that will be somewhat more difficult to change later. Otherwise, I would be happy to take another look at the code and then merge this pull request.

[progfolio]

Perhaps :compile or :system or :shell or something of that nature would be more explanatory. I think :build is just fine as well, and you are right that the message can be changed regardless.

I think sticking with the :build keyword is fine for the recipe format and then distinguishing that part of the process in the documentation/messaging will be fine.
I'll think on it as far as what message to emit during the process.

For the :build portion of the recipe, I've used the symbol default in the alist to signify a fallback if system-name doesn't match any specific set of instructions.
This simplifies logic for getting the :build instructions. e.g.

( :package "Foo"
  :build ((berkeley-unix . (("gmake") ("etc")))
          (default       . (("make")  ("etc")))))

I've also added the ability to eval elisp in the :build instructions and/or provide a backqouted list for programmatic :build instructions.
This brings us closer to el-get compatibility and simplify recipes. e.g.

( :package "Foo"
  :build `((default . ((,(concat (when (eq system-type 'berkeley-unix) "g") "make") (message "Foo built"))))))

What if, instead of running the build commands in the build directory, we run them in the source directory, before symlinking? Then, anything related to make will automatically be fast because of the dependency tracking. I think this is actually how el-get itself works (i.e., it runs commands directly in the repository).

Unless I'm misunderstanding, the :build step is already run in the repo's directory.
straight--run-build-commands has a local variable, build-dir, but it was referring to the :build process.
I've changed it to repo to make that distinction clearer and avoid the overloaded build terminology discussed above.
The call to straight--run-build-commands is just before straight--symlink-package in straight--build-package.
Though, I am still having trouble with symlinking org-version.el after building Org, so I suspect there is more at play.
I thought :defaults in the file directive would take care of symlinking lisp/org-version.el, but it apparently does not.
I see that straight--expand-files-directive-internal filters nonexistent files, but if we're building before symlinking, this shouldn't be a problem.
So I may be misunderstanding something about the order of things if you have any pointers there.

Perhaps, in the future when support for el-get is added to straight.el, we can add the option for .elc files to be linked into the build directory from the source repository, and then byte-compilation for those files could be skipped. This would be the best possible performance, I think.

Yes. I definitely think we'll have to iron out some cases where straight and the recipe do the same work. Your proposal sounds like a good idea.
The same would go for info docs, if possible.
Some makefiles build/install those as well.

Regarding security:

…an ecosystem problem

sums it up rather well.
I still think it's something worth attempting, but out of the scope of this pull, so I'll shelve the idea for now.
If I think of a way to provide a reasonable amount of security or encourage more secure habits, I'll propose it in a separate issue.

[raxod502]

I've also added the ability to eval elisp

Great! Although, why is there a backquote? Why not only the unquote (comma) to indicate that unlike the rest of the recipe, this one part should be evaluated?

the :build step is already run in the repo's directory.

Oh, okay -- sorry. Sounds good then.

I thought :defaults in the file directive would take care of symlinking lisp/org-version.el, but it apparently does not.

That's odd, I don't know why that would be. You should be able to run straight-expand-files-directive manually though and see exactly what it comes up with for your setup.

If I think of a way to provide a reasonable amount of security or encourage more secure habits, I'll propose it in a separate issue.

Please do, indeed! I would love to see improvements in this area!

[progfolio]

Great! Although, why is there a backquote? Why not only the unquote (comma) to indicate that unlike the rest of the recipe, this one part should be evaluated?

The :build keyword's value is evaluated if it starts with a backquote when it is initially read.
This allows one to specify :build commands programmatically.
Otherwise each command would have to be pure elisp or strings specifying shell commands,
or the entire recipe would need to be backquoted and evaluated.

I based the approach off of the pattern I saw in el-get recipes which utilize the :build* keywords.
e.g. Org-mode:

( :name org-mode
  :website "http://orgmode.org/"
  :description "Org-mode is for keeping notes, maintaining ToDo lists, doing project planning, and authoring with a fast and effective plain-text system."
  :type git
  :url "https://code.orgmode.org/bzg/org-mode.git"
  :info "doc"
  :build/berkeley-unix `,(mapcar
                          (lambda (target)
                            (list "gmake" target (concat "EMACS=" (shell-quote-argument el-get-emacs))))
                          '("oldorg"))
  :build `,(mapcar
            (lambda (target)
              (list "make" target (concat "EMACS=" (shell-quote-argument el-get-emacs))))
            '("oldorg"))
  :load-path ("." "contrib/lisp" "lisp")
  :load ("lisp/org-loaddefs.el"))

or helm:

(:name helm
       :description "Emacs incremental completion and narrowing framework"
       :type github
       :pkgname "emacs-helm/helm"
       :autoloads "helm-autoloads"
       :build `(("make"
                 ,(concat "ASYNC_ELPA_DIR=" (el-get-package-directory 'emacs-async))))
       :depends (emacs-async)
       :build/darwin `(("make"
                        ,(concat "ASYNC_ELPA_DIR=" (el-get-package-directory 'emacs-async))
                        ,(format "EMACS_COMMAND=%s" el-get-emacs)))
       ;; Windows probably doesn't have make available so we fake it.
       :build/windows-nt
       (let ((generated-autoload-file (expand-file-name "helm-autoloads.el")) \
             (backup-inhibited t))
         (update-directory-autoloads default-directory)
         nil)
       :build/berkeley-unix `(("gmake"
                               ,(concat "ASYNC_ELPA_DIR=" (el-get-package-directory 'emacs-async))))
       :features "helm-config"
       :post-init (helm-mode))

When it comes time to add el-get support, we can just check for a leading backquote on any of the :build* keywords when mapping them to an alist and backquote the whole
list if one is present. e.g.
Helm translated to straight's notion of :build:

( :name helm
  ;;etc
  :build `((default . (("make" ,(concat "ASYNC_ELPA_DIR=" (el-get-package-directory 'emacs-async)))))
           (darwin  . (("make" ,(concat "ASYNC_ELPA_DIR=" (el-get-package-directory 'emacs-async))
                        ,(format "EMACS_COMMAND=%s" el-get-emacs))))
           ;; Windows probably doesn't have make available so we fake it.
           (windows-nt . ((let ((generated-autoload-file (expand-file-name "helm-autoloads.el"))
                                (backup-inhibited t))
                            (update-directory-autoloads default-directory)
                            nil)))
           (berkeley-unix . (("gmake" ,(concat "ASYNC_ELPA_DIR=" (el-get-package-directory 'emacs-async))))))
  ;;etc
  )

[raxod502]

Okay! I was skeptical at first, but it has grown on me. One thing that is particularly nice about your solution is that if you want to backquote/unquote other parts of the recipe to generate part of it dynamically, that won't interfere with the backquote/unquote in the :build part, because the outer backquote shields the inner one: ``,default-directory evaluates to `,default-directory instead of `"some string".

[progfolio]

Good news re: symlinking. I believe the problem was with my config.
elfeed-org was checking org-version before it was available.
Deferred its loading via use-package's :after keyword and now the symlink is working fine.

Made a couple of small changes to straight--run-build-commands:

Instead of using the default key in the alist, the default is now just a list of commands. I wanted to do this initially, but thought it would be harder to implement than it was.
So now, if system-type does not match an element in the alist, we search for and return the first element with a car that is not a symbol.
So long as the alist is otherwise well-formed, this should work fine.

I've also made it so the cdr of the alist may map to a single command or a list of commands.
Again, not a major change, but is syntactically better IMO.

An example demonstrating both

Old syntax:

(straight--run-build-commands
 '( :package "example"
    :build ((default   . (((message "configuring...") (message "building...") (message "etc."))))
            (gnu/linux . ((message "single build command"))))))

Current syntax:

(straight--run-build-commands
 '( :package "example"
    :build (((message "configuring...") (message "building...") (message "etc."))
            (gnu/linux . (message "single build command")))))

[raxod502]

Well, I like that the second argument doesn't have to be a list of commands, but why is it better for the default entry to look different than the others? I can see why it would be good to allow the entire :build spec to just be a list of commands or single command, if the default entry is the only one, but supposing there is an alist the new way seems harder to read to me. Perhaps I am missing something?

[raxod502]

looks great to me! (the newly added documentation)

[progfolio]

Anything else you think this needs?

[raxod502]

No, I don't think so! :)

[raxod502]

And the legendary saga comes to an end! Thanks so much for your extensive work on this.

[aspiers]

Congrats guys, this is awesome!

[shackra]

Glad to see this! Congratulations

No, I don't think so! :)

Well, I do believe that a documented example of how to solve the issue with org-mode using the new facilities should be added to the readme. I have something like this:

(setq straight-fix-org nil)
(use-package org
  :straight (org-plus-contrib
             :type git
             :repo "https://code.orgmode.org/bzg/org-mode.git"
             :local-repo "org"
             :build ((gnu/linux . (("make" "oldorg" "EMACS=/usr/bin/emacs"))))
             :files (:defaults "contrib/lisp/*.el")
             :includes (org))
;;; ... a bunch of code ...
)

and, I'm getting this error every time I want to do something in Emacs:

Problems while trying to load feature ‘org-bbdb’
Problems while trying to load feature ‘org-bibtex’
Problems while trying to load feature ‘org-docview’
Problems while trying to load feature ‘org-gnus’
Problems while trying to load feature ‘org-info’
Problems while trying to load feature ‘org-irc’
Problems while trying to load feature ‘org-mhe’
Problems while trying to load feature ‘org-rmail’
Problems while trying to load feature ‘org-w3m’
File mode specification error: (error Autoloading file /home/jorge/.emacs.d/straight/build/org-plus-contrib/org.elc failed to define function org)
Problems while trying to load feature ‘org-bbdb’
Problems while trying to load feature ‘org-bibtex’
Problems while trying to load feature ‘org-docview’
Problems while trying to load feature ‘org-gnus’
Problems while trying to load feature ‘org-info’
Problems while trying to load feature ‘org-irc’
Problems while trying to load feature ‘org-mhe’
Problems while trying to load feature ‘org-rmail’
Problems while trying to load feature ‘org-w3m’
File mode specification error: (error Autoloading file /home/jorge/.emacs.d/straight/build/org-plus-contrib/org.elc failed to define function org)
[yas] Prepared just-in-time loading of snippets successfully.
Problems while trying to load feature ‘org-bbdb’
Problems while trying to load feature ‘org-bibtex’
Problems while trying to load feature ‘org-docview’
Problems while trying to load feature ‘org-gnus’
Problems while trying to load feature ‘org-info’
Problems while trying to load feature ‘org-irc’
Problems while trying to load feature ‘org-mhe’
Problems while trying to load feature ‘org-rmail’
Problems while trying to load feature ‘org-w3m’
No event to add
map-put!: Autoloading file /home/jorge/.emacs.d/straight/build/org-plus-contrib/org.elc failed to define function org
Explain-pause-mode enabled.
Problems while trying to load feature ‘org-bbdb’
Problems while trying to load feature ‘org-bibtex’
Problems while trying to load feature ‘org-docview’
Problems while trying to load feature ‘org-gnus’
Problems while trying to load feature ‘org-info’
Problems while trying to load feature ‘org-irc’
Problems while trying to load feature ‘org-mhe’
Problems while trying to load feature ‘org-rmail’
Problems while trying to load feature ‘org-w3m’
Error during redisplay: (dashboard-resize-on-hook #<frame *dashboard* - GNU Emacs at sanson-gnu 0x56108ffaea70>) signaled (error "Autoloading file /home/jorge/.emacs.d/straight/build/org-plus-contrib/org.elc failed to define function org")
LSP :: There are no language servers supporting current mode `emacs-lisp-mode' registered with `lsp-mode'.
This issue might be caused by:
1. You haven't registered/loaded external language server package which doesn't ship with `lsp-mode'(e. g. `lsp-java', `lsp-metals').
2. The language server that you expect to run is not configured to run for major mode `emacs-lisp-mode'. You may check that by checking the `:major-modes' that are passed to `lsp-register-client'.
3. `lsp-mode' doesn't have any integration for the language behind `emacs-lisp-mode'. Refer to https://emacs-lsp.github.io/lsp-mode/page/languages and https://langserver.org/ .
Problems while trying to load feature ‘org-bbdb’
Problems while trying to load feature ‘org-bibtex’
Problems while trying to load feature ‘org-docview’
Problems while trying to load feature ‘org-gnus’
Problems while trying to load feature ‘org-info’
Problems while trying to load feature ‘org-irc’
Problems while trying to load feature ‘org-mhe’
Problems while trying to load feature ‘org-rmail’
Problems while trying to load feature ‘org-w3m’
run-hooks: Autoloading file /home/jorge/.emacs.d/straight/build/org-plus-contrib/org.elc failed to define function org
Problems while trying to load feature ‘org-bbdb’
Problems while trying to load feature ‘org-bibtex’
Problems while trying to load feature ‘org-docview’
Problems while trying to load feature ‘org-gnus’
Problems while trying to load feature ‘org-info’
Problems while trying to load feature ‘org-irc’
Problems while trying to load feature ‘org-mhe’
Problems while trying to load feature ‘org-rmail’
Problems while trying to load feature ‘org-w3m’
Error during redisplay: (dashboard-resize-on-hook #<frame *dashboard* - GNU Emacs at sanson-gnu 0x56108ffaea70>) signaled (error "Autoloading file /home/jorge/.emacs.d/straight/build/org-plus-contrib/org.elc failed to define function org")
Mark set 

[aspiers]

@shackra See this comment on #568 - my understanding is that this will be fixed out of the box by stealing from el-get's org-mode.rcp which already has the relevant :build section.

[shackra]

okay, my issue had nothing to do with installing org, it was the :hook thingy in my use-package declaration that was wrong. Having this instead fixes my problem.

(use-package org
  :straight org-plus-contrib
  :hook ((org-mode . yas-minor-mode-on)
         (org-mode . org-mode-visual-fill))
;; ... bunch of code
)

[progfolio]

@shackra Glad to see that issue is resolved. I have seen a few people with :includes (org) in their recipes for org-plus-contrib, but I don't think this was ever implemented as a keyword. It was floated as an idea here:

#352 (comment)

But the recipe straight provides for org-plus-contrib should cover it by setting the :local-repo to "org".

[raxod502]

If Org doesn't work out of the box with either of these forms:

(straight-use-package 'org)
(straight-use-package 'org-plus-contrib)

then it's a bug, since straight.el provides its own recipe for Org via the org-elpa virtual recipe repository. Since this pull request was merged, the straight.el Org recipe includes a :build option which should make things work correctly.

[zoechi]

(straight-use-package 'org)
(straight-use-package 'org-plus-contrib)

How should this look look like with (use-package ...)?

[raxod502]

This is documented in https://github.com/raxod502/straight.el#integration-with-use-package-1.

By default, straight.el installs a new keyword :straight for use-package which may be used to install packages via straight.el. The algorithm is extremely simple. This:

(use-package el-patch
  :straight t)

macroexpands (essentially) to:

(straight-use-package 'el-patch)

[zoechi]

@raxod502 why are some using #624 (comment)

(use-package org
        :straight org-plus-contrib

I can't figure out how I could derive that from the docs you cited above.

[progfolio]

relevant part of that section of the docs:

If the feature you are requiring with use-package is different from the package name, you can provide a full recipe:

(use-package tex-site
:straight (auctex :host github
:repo "emacsmirror/auctex"
:files (:defaults (:exclude "*.el.in"))))

And you may also provide just the package name:

(use-package tex-site :straight auctex)

In the example you gave, people are installing the development version of org + contrib files, but the feature provided by that package is still called org:

(use-package org
  :straight org-plus-contrib)

expands to:

(progn
  (straight-use-package 'org-plus-contrib)
  (require 'org nil nil))

[raxod502]

See also https://github.com/joddie/macrostep, which I find very helpful in answering questions about how macros expand.