search

melpa / package-build

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

package-build.el: determine package-build-timeout-executable lazily #29

Closed Fuco1 closed 5 years ago

Fuco1 commented 5 years ago

Fixes #24

I'm willing to fix all the bugs and issues related to this change.

This change will make life of all the cask users a lot better.

(cc https://github.com/cask/cask/issues/361)

purcell commented 5 years ago

Thanks for this! I'm sorry it has been a problem for cask.

Thinking about it, my preferred solution would be to just set the var to "timeout" by default, and set package-build-timeout-secs to nil by default. That way, users would have to opt in to the timeout behaviour, and explicitly configure it. Even the executable-find at load time is bad practice anyway, because there's no guarantee about whether load-path has been customised before any particular file is loaded.

Would that also solve the problem for you? It's not clear to me whether cask would actually want to use the timeout behaviour in package-build.

Fuco1 commented 5 years ago

@purcell Hey, no worries! All this stuff is pretty complex from every angle, so let's not rush it :)

The main gripe here is that when cask is called on the command line it initializes this quite unnecessarily, because not every time you are going to build packages. For example cask exec sets up the exec path and runs emacs or anything in that environment, not touching the build process.

I'm actually not sure if cask uses this variable during build, but it probably has been since it was by default set to some binary.

However since cask also aims to support windows platform I'm pretty sure it is just fine without using the timeout feature. It could maybe cause some lock-ups in CI builds and such but there the platforms usually implement timeouts on top of the jobs anyway so that's not a problem.

purcell commented 5 years ago

Cool, I've gone ahead and done that. The MELPA scripts now explicitly set a timeout of 600s (via package-build-timeout-secs), and unless other client code like cask does the same, no timeouts will be applied, and no "timeout" executable will be required. Ping me if you find any issues!

Fuco1 commented 5 years ago

@purcell thank you, will do!

tarsius commented 5 years ago

@Fuco1 Do you have commit access to Cask?

If so, then please make 9aac3517bde14346eadbe2c7d354672a456b9db3 unnecessary.

Fuco1 commented 5 years ago

@tarsius I do indeed. I'll create an issue in cask to track it.