Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The :depends keyword does not take package ownership into account #9134

Closed
Stebalien opened this issue Jun 23, 2017 · 19 comments
Closed

The :depends keyword does not take package ownership into account #9134

Stebalien opened this issue Jun 23, 2017 · 19 comments
Assignees
Labels
- Bug tracker - Bug :-( Fixed in develop stale marked as a stale issue/pr (usually by a bot)

Comments

@Stebalien
Copy link
Contributor

Description :octocat:

The layer packages :depends keyword does not take package ownership into
account. That is, if a layer declares a package in <layer>-packages but
doesn't initialize it (doesn't own it), the :depends keyword will still treat
the package as if it were available.

For example, given:

layers/foo/packages.el:

(setq foo-packages '(foo))

(defun foo/pre-init-foo ())

layers/bar/packages.el:

(setq bar-packages '(bar :depends foo))

(defun bar/init-bar ())

bar will be installed (and foo along with it if the bar package actually
depends on foo).

System Info 💻

  • OS: gnu/linux
  • Emacs: 26.0.50
  • Spacemacs: 0.200.9
  • Spacemacs branch: local (rev. c5ef8edd)
  • Graphic display: t
  • Distribution: spacemacs-base
  • Editing style: vim
  • Completion: ivy
@TheBB
Copy link
Collaborator

TheBB commented Jun 23, 2017

But in this case the bar layer owns the bar package. The :depends property is checked for packages which a layer doesn't own.

@TheBB
Copy link
Collaborator

TheBB commented Jun 23, 2017

Just realized that it needs to do that, too.

TheBB added a commit to TheBB/spacemacs that referenced this issue Jun 23, 2017
@TheBB TheBB self-assigned this Jun 23, 2017
@TheBB
Copy link
Collaborator

TheBB commented Jun 23, 2017

@Stebalien @skebanga Can you try #9136

@steve-lorimer
Copy link
Contributor

First time I've done this, so not sure if it's correct, although I believe it is (followed instructions here)

$ git fetch origin pull/9136/head:fix-9134
$ git checkout fix-9134

Restarted spacemacs and the warnings went away

Reverted to develop

$ git checkout develop

Restarted spacemacs and the warnings were back.

So for me, looks like it's workin! Thanks @TheBB

@Stebalien
Copy link
Contributor Author

Hm. Maybe I'm experiencing a different issue? My problem is that helm is getting installed because:

  1. The shell layer declares it but doesn't own it.
  2. Several other layers pull packages that depend on helm with (helm-something :depends helm).

@TheBB
Copy link
Collaborator

TheBB commented Jun 24, 2017

I'll solve this but it may be a few days.

@TheBB
Copy link
Collaborator

TheBB commented Jul 1, 2017

I have updated the PR. @Stebalien I think it should fix your problem. Please let me know.

@choppsv1
Copy link
Contributor

choppsv1 commented Jul 1, 2017

With the latest changes I am able to re-enable ivy (disabling helm) and cleanly launch rather than failing in eieio...

@jsoo1
Copy link

jsoo1 commented Jul 1, 2017

I also tested on fix-9134. Looks pretty good! 👍 Thanks!

@Stebalien
Copy link
Contributor Author

Still broken for me. Describe package gives:

helm is an installed package.

     Status: Installed in ‘helm-20170628.2213/’ (unsigned).
    Version: 20170628.2213
    Summary: Helm is an Emacs incremental and narrowing framework
   Requires: emacs-24.4, async-1.9.2, popup-0.5.3, helm-core-2.8.0
Required by: restclient-helm-20170314.854, helm-hoogle-20161026.2234, helm-gitignore-20170210.1608, helm-css-scss-20140626.1725, helm-company-20170608.1029,
               helm-c-yasnippet-20170128.742
   Homepage: https://emacs-helm.github.io/helm/
Other versions: 20170628.2213 (melpa).
  1. All of the "Required by" packages have :depends helm specified and aren't dependencies of any other package.
  2. I don't have the helm layer enabled so nothing else should be pulling in helm. The only layer I have that mentions helm directly is the shell layer but that layer doesn't "own" the helm package.

@Stebalien
Copy link
Contributor Author

I've tried removing the helm package configuration from the shell layer and uninstalling all helm related packages but they just get re-installed when I restart.

So, maybe this doesn't have anything to do with "unowned" packages? The one thing that fixes this for me to change the shell layer's helm dependency to:

        (helm :toggle (configuration-layer/layer-usedp 'helm))

That results in a warning about the shell layer not owning the helm package but also prevents the helm package from getting installed.

However, it appears that simply removing the helm dependency from that package doesn't prevent it from being installed.

@TheBB
Copy link
Collaborator

TheBB commented Jul 1, 2017

Is the problem that the helm package is being downloaded or that it's being configured and enabled?

@Stebalien
Copy link
Contributor Author

Helm is only being downloaded, not configured (although it still shouldn't be downloaded in this case). However, the other packages (the ones that declare :depends helm) are being configured (e.g., helm-gitignore).

@TheBB
Copy link
Collaborator

TheBB commented Jul 2, 2017

With the latest version of my PR I can't verify the latter problem you have. I get

(Spacemacs) helm-gitignore is ignored since it has dependencies that are not used.

@TheBB
Copy link
Collaborator

TheBB commented Jul 2, 2017

Sorry, I accidentally posted too soon. Here's the real comment.

@Stebalien I have updated the PR again. Please have a look.

Depending on your exact config, you will probably find that the helm package is still being installed, because the following packages (at least, there may be more) depend on it (not in the Spacemacs :depends sense of "don't install this package if its dependencies are not present" but in the package.el sense of "to install this package, install these other ones too"):

  • counsel-dash in the dash layer depends on helm-dash which depends on helm
  • emoji-cheat-sheet-plus in the emoji layer depends on helm directly
  • helm-make in the ivy layer (!) depends on helm directly
  • org-ref in the bibtex layer depends on helm-bibtex which depends on helm

If you found that the helm package was not installed previously and that any of these four packages (or some I have not discovered) were used in your config, I can only assume that was a bug that has been fixed, because we cannot in good conscience override the dependencies that package authors have decided to use.

Now, whether any of these packages need to be declared with :depends helm or not is an open question. They were not declared with :toggle (configuration-layer/package-usedp 'helm) before.

@TheBB
Copy link
Collaborator

TheBB commented Jul 2, 2017

I'll rename :depends to :requires to avoid confusion between the two notions. :-)

TheBB added a commit to TheBB/spacemacs that referenced this issue Jul 2, 2017
syl20bnr pushed a commit that referenced this issue Jul 2, 2017
@syl20bnr
Copy link
Owner

syl20bnr commented Jul 2, 2017

Should be fixed in develop.
@TheBB +1 for the renaming.

@Stebalien
Copy link
Contributor Author

@TheBB The latest version (the one merged into develop) works. Thanks!

(FYI, I had already already excluded those packages for precisely this reason.)

jasoncyu pushed a commit to jasoncyu/spacemacs that referenced this issue Sep 30, 2017
CodingSolo pushed a commit to CodingSolo/spacemacs that referenced this issue Oct 29, 2018
@github-actions
Copy link

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Please let us know if this issue is still valid!

@github-actions github-actions bot added the stale marked as a stale issue/pr (usually by a bot) label Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- Bug tracker - Bug :-( Fixed in develop stale marked as a stale issue/pr (usually by a bot)
Projects
None yet
Development

No branches or pull requests

6 participants