package-build--package name change broke cask

dajva commented 6 years ago

Hi, This is related to the recent refactories by @tarsius. The change that renamed package-build-package broke cask again. See https://github.com/cask/cask/issues/426

This function being private is a bit problematic for cask since it depends on this in multiple places. It could still be used from cask anyway of course, but it's not good practice to use private functions in other packages. With the recent refactorings in mind the line between private and public functions might be academic for package-build but my impression is that this instability in the API is temporary and will be better in the future, right?

Is there any public alternative for this functionality that can be used instead?
Would you consider making it public again to support the casks use case? It's a huge benefit IMO that cask and melpa are using the same code for creating the packages since that means you can verify and debug your package locally.

@sambrightman: What do you say about this?

tarsius commented 6 years ago

Having looked at this for a few minutes, I would say that the best short-time course of action is to simply follow the rename.

If we make this function public again or provide a public wrapper, then we should probably make it more convenient for callers at the same time. For example does Cask actually benefit from the existence of package-build-archive-dir? Or does it just let-bind that around calls to function that use it because that's how that piece of information is passed around (instead of using function arguments)?

dajva commented 6 years ago

Thanks for the quick response. Unfortunately my experience with the cask code base is very restricted so I can't help much with improving such an API. I will have a look though to see if I can figure how this is actually used and maybe give some hints. For now I will submit a pr to cask for using the private function.

tarsius commented 6 years ago

The main difference between package-build-archive and package-build--package is that the former calculates the version internally using (package-build--checkout rcp). This is an issue because Cask doesn't map package name => package recipe the same way as package-build. Where package-build has a definite collection of available packages, Cask uses its own representation for a dependency and turns that into a recipe pretty late in cask--checkout-and-package-dependency, using information that isn't available to package-build.el and package-build-archive in particular.

dajva commented 6 years ago

For example does Cask actually benefit from the existence of package-build-archive-dir? Or does it just let-bind that around calls to function that use it because that's how that piece of information is passed around (instead of using function arguments)?

For this particular question I can say (since I put it there) that it's just used because that's the available method to pass that piece of information. The old API, before the first part of the refactorings landed, used an argument which is more natural from cask's perspective.

tarsius commented 6 years ago

Also see https://github.com/melpa/package-build/pull/16#issuecomment-395935948.

dajva commented 6 years ago

Thanks, FWIW I think your suggestion for an API in that comment looks reasonable. I don't have any association with cask other than submitting these bandage fixes so I can't speak for them. Unfortunately there seems to be very little activity there so progress is very slow.

I did however look a bit into how these API:s are used from cask. For the package-build--package related functionality there are two use cases.

Dependencies - cask dependencies are converted to package-build recipes and package-build--checkout and package-build--package are used to create a package.
Package development - Building the package under development from current directory. Support for this was solved by creating a subclass package-directory-recipe of the package-recipe class overriding some key methods and then using package-build--package.

Merging these two use cases into a single function that checkouts the source and builds it would get rid of the two internal functions that cask is using currently. For the directory version, checkout would have to be overridden as a no-op but I think it should work fine and would reduce the coupling between these packages.

Other package-build constructs used from cask:

package-build-expand-file-specs
package-build-default-files-spec
package-build-verbose

These are all "public" so I assume this is fine. As I said, these are my personal thougts and suggestions. You would need confirmation that this is a good approach from some official downstream people before doing any changes according to this.

Wilfred commented 6 years ago

@tarsius would you be willing to apply your kludge in e2acbd9f5f38286dcbb32015f718f3c94389820d to package-build until cask has moved to the new API?

The cask PR https://github.com/cask/cask/pull/429 has been open for 22 days, so I think cask moves pretty slowly and it's a shame that new users can't use cask without manually applying that PR.

tarsius commented 6 years ago

Okay, I have merged it and created a new release.

Wilfred commented 6 years ago

Thank you :)

melpa / package-build

package-build--package name change broke cask #20