More flexible additional packages

[TheBB]

Edit: This PR has been through a couple of design iterations, so the discussion may not make sense. The OP is current.

This might be a little contentious, but I had some spare time and figured that it wouldn't be so difficult to do... so...

Upstream sometimes often breaks. Even though we have made great leaps towards package version pinning, we're still a good ways off from doing it efficiently on a large scale. On the other hand, it's easy to do locally on a user level: just edit the relevant packages.el with a recipe. Then again, any solution for end users that involves editing the Spacemacs source code smells illegitimate to me.

This streamlines the approach by using configuration-layer/make-package when going through dotspacemacs-additional-packages to copy information into the existing package object if it exists. This effectively means that the location, step and excluded slots become overridable, but the owner, pre-layers and post-layers slots do not.

Details

This also adds a new valid location attribute called local-dotfile. This is an implementation detail, to distinguish local packages (in layers) from packages which have their location overridden to local. This is based on the assumption that the source code in the latter case should not be found in a subdirectory of the owning layer, but somewhere else entirely. For those packages, .emacs.d/local/<pkg-name> will be added to the load path.

Unintended goodies

In its current state, this PR allows us to deprecate dotspacemacs-excluded-packages since the same effect can be obtained with (pkg-name :excluded t) in dotspacemacs-additional-packages, which nicely mirrors how layer package lists work.

[syl20bnr]

I hear you, currently overriding a location is very cumbersome. I think that with a little hack we could use dotspacemacs-additional-packages. I would go this route instead of the one with a new variable, it will be less code and will work generaly with all the overridable keywords. Then maybe we should start to think about renaming this variable but dotspacemacs-additional-packages would be OK to me if we cannot find something better, the important thing is the docstring and documentation.

[TheBB]

Actually, that wouldn't be difficult at all. The additional packages are already loaded last. All we have to do is add a check for :keep-owner or some such property before setting the owner.

[TheBB]

Updated with new suggestion. It's quite a bit simpler.

[syl20bnr]

Is the new location required ? I find it confusing if it is required for what we want to achieve here. I would have worked at the owner level instead https://github.com/TheBB/spacemacs/blob/override-locations/core/core-configuration-layer.el#L338, i.e. if there is already a owner then don't override it to dotfile owner.

[syl20bnr]

I don't see why :keep-owner is required too, if we just prevent the code from overwriting any already owned package for dotfile then we are good or maybe I'm missing something.

[TheBB]

No, the location is optional. I was just trying to not break any existing configs: if you add helm (say) as an additional package the owner will be changed. But we can break backwards compatibility, if you want.

[TheBB]

I have updated again. See OP for how this currently works.

[TheBB]

You understand the intricacies of core much better than I do. I had some problems when trying this out on a couple of packages, but maybe I was just doing it wrong. I think the basic mechanism is correct, though.

[syl20bnr]

Your implementation for the owner is flawless. I removed the part for local-dotfile because we just have to check if the package has a local location here: https://github.com/syl20bnr/spacemacs/blob/develop/core/core-configuration-layer.el#L746-L749
The condition for local location with a regular layer comes after so it won't be executed. Feel free to submit a new PR with the correct local path for packages owned by the dotfile.
As I wrote, flawless PR to be able to override the properties 👍
Thank you!
Cherry-picked, modified according to the comment above and squashed into develop, you can safely delete your branch.

[TheBB]

Great. I'm not sure I get it though.

If a layer has a package whose location is local, the load path will be <layer>/local/<package>/. If I override a package to be local, I probably don't want to put it there. By the time configuration-layer/configure-packages-2 comes around, there's no way to tell the difference between a package that was configured to be local by the layer and a package that was configured to be local by me, right?

Of course, what we can do is just to add some other path to the load path for all local packages, since a missing path is not a problem.

[syl20bnr]

I think this is a rule to add in the same function as what was merged: if the overriden (no need to be overriden) :location is local then we put the owner to be dotfile. Then you can do what I described above.

[syl20bnr]

Thinking about it, it is better to make it explicit with a new location but I would not bring the dotfile in the location name, we could use the private directory.

So I propose to add a :location private (no need to tell that it is local, I like symmetry with 1 word for all options).

Sorry to make you re-implement the same kind of stuff...

[syl20bnr]

But in practice people will use a github recipe to achieve this ;-) But it is a good option to have, nice catch 👍

[TheBB]

Gotcha, I like the private solution.