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

Fix packup to correctly handle melpa-stable, and other fixes #108

Merged
merged 11 commits into from
Jul 15, 2018

Conversation

cg505
Copy link
Contributor

@cg505 cg505 commented Jul 7, 2018

That's right: packup no longer tries to install the latest melpa even though you told it to use melpa-stable! Woo!

Other things I like are added. Feel free to complain if you don't like them.

This resolves #99.

@@ -10,7 +10,7 @@

(package-initialize)

(defvar kotct/packup-marker-char ?x
(defvar kotct/packup-marker-char ?*
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I kept seeing x as the mark, so my brain would go to mark something and I'd hit x on my keyboard. Then it would execute the transaction. Changing to * avoids this issue and also brings the marks more in line with the rest of emacs (think dired, list-buffers, etc).

rye
rye previously requested changes Jul 7, 2018
Copy link
Member

@rye rye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, just a couple of English-related nits.

Does not automatically refresh package list."
(every (lambda (x) (package-installed-p package (package-desc-version x)))
(cdr (assq package package-archive-contents))))
"Returns T if PACKAGE is installed and up-to-date.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, it should probably just be "non-nil", since that's what package-installed-p says in its docstring.

(cdr (assq package package-archive-contents))))
"Returns T if PACKAGE is installed and up-to-date.
Does not automatically refresh package list.
Before emacs 25, we have to manually check in preferred-repository order."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emacsEmacs?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup

@cg505
Copy link
Contributor Author

cg505 commented Jul 7, 2018

Since this PR does a LOT. I would appreciate it if you could checkout and try some things on the branch. You should be able to delete your .emacs.d/elpa directory entirely and get almost all packages from melpa-stable.

@cg505 cg505 dismissed rye’s stale review July 7, 2018 18:20

addressed comments and made some other docstring changes

@cg505 cg505 requested a review from rye July 7, 2018 18:20
@rye
Copy link
Member

rye commented Jul 7, 2018

Okay, I did that. I was able to install everything I needed.

One thing is that we should make sure that packup-update doesn't try to update from a stable version to a non-stable version, which it seems to do. (Try deleting ~/.emacs.d/elpa, starting Emacs, answering y for everything, and then observe that M-x kotct/packup-update prompts you to install "newer" versions.)

EDIT: I was on master. Boo. [EDIT 2:] I get the same behavior on packup-fixes, too. I'll try nuking .elc files to make sure it's not that. [EDIT 3:] It was my .elc files not getting cleaned. This problem no longer exists.

For reference, here's the package.el source that corresponds to the U behavior in package-list-packages.

@cg505
Copy link
Contributor Author

cg505 commented Jul 7, 2018

Yeah, we should essentially end up with the same result as using U in the packages buffer. The only lingering issue, I think is that transitive dependencies might not have updates triggered. This is a non-trivial fix, and I'll need to spend a while figuring it out. I don't really want to hold this up with that.

So... did it work on the real branch?

@rye
Copy link
Member

rye commented Jul 7, 2018

Yes, I edited my comment a few times to match the current state of my hunt. After nuking .elc files, I ended up getting intended results. (U on package-list-packages reports no upgrades and M-x kotct/packup-update doesn't report any upgrades.)

Copy link
Member

@rye rye left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After poking around, I'm happy with using this in production.

@cg505
Copy link
Contributor Author

cg505 commented Jul 7, 2018

I am surprised nuking .elc files had an impact. We should always prefer the newer file between .el and .elc. It could be an issue with how we autoload. Probably requires further investigation in a separate issue.

I want to leave this open for at least a couple days in case @samontea has a chance to glance at it (since he wrote a lot of the code that was modified).

Copy link
Contributor

@samontea samontea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@cg505
Copy link
Contributor Author

cg505 commented Jul 15, 2018

@samontea 😍

@cg505 cg505 merged commit 3147ffb into master Jul 15, 2018
@rye rye mentioned this pull request Jul 15, 2018
@rye rye added this to the Version 0.2.0 milestone Jul 15, 2018
@rye rye deleted the packup-fixes branch July 16, 2018 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

packup does not respect package-archive-priorities
3 participants