* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
[not found] ` <E1YC44Z-0002sG-4a@vcs.savannah.gnu.org>
@ 2015-01-16 19:10 ` Glenn Morris
2015-01-16 19:33 ` Jorgen Schäfer
2015-01-16 23:48 ` Artur Malabarba
1 sibling, 1 reply; 15+ messages in thread
From: Glenn Morris @ 2015-01-16 19:10 UTC (permalink / raw)
To: emacs-devel; +Cc: Jorgen Schaefer
Thanks for writing a test case. However, it fails:
http://hydra.nixos.org/build/18817665
(wrong-type-argument arrayp ([cl-struct-package-desc ... ))
Also, you forgot to make a test/ChangeLog entry.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-16 19:10 ` [Emacs-diffs] master b689b90: Package archives now have priorities Glenn Morris
@ 2015-01-16 19:33 ` Jorgen Schäfer
2015-01-16 20:09 ` Glenn Morris
2015-01-19 21:40 ` Glenn Morris
0 siblings, 2 replies; 15+ messages in thread
From: Jorgen Schäfer @ 2015-01-16 19:33 UTC (permalink / raw)
To: Glenn Morris; +Cc: emacs-devel
On Fri, Jan 16, 2015 at 8:10 PM, Glenn Morris <rgm@gnu.org> wrote:
>
> Thanks for writing a test case. However, it fails:
>
> http://hydra.nixos.org/build/18817665
>
> (wrong-type-argument arrayp ([cl-struct-package-desc ... ))
Thanks. I had serious trouble getting the test suite to run locally,
so I tried to make it work with manual execution. Seems I failed at
that.
What's the correct way of invoking the test suite? "make check"
results in 46 test failures here.
Also, can I run individual tests from the command line easily?
Especially the helper macros in package-test.el seem a bit fickly with
interactive use.
> Also, you forgot to make a test/ChangeLog entry.
Oh, there's another ChangeLog file.
I hope 78e6ccc fixes the test and adds the commit message.
Regards,
Jorgen
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-16 19:33 ` Jorgen Schäfer
@ 2015-01-16 20:09 ` Glenn Morris
2015-01-19 21:40 ` Glenn Morris
1 sibling, 0 replies; 15+ messages in thread
From: Glenn Morris @ 2015-01-16 20:09 UTC (permalink / raw)
To: Jorgen Schäfer; +Cc: emacs-devel
Jorgen Schäfer wrote:
> What's the correct way of invoking the test suite? "make check"
> results in 46 test failures here.
make check is correct.
Sadly the test-suite often seems to be in a failing state.
I don't have anything like 46 failures though.
I can only suggest making bug reports.
> Also, can I run individual tests from the command line easily?
See the top of the Makefile. Eg
make -C test/automated package-test
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: master b689b90: Package archives now have priorities.
[not found] ` <E1YC44Z-0002sG-4a@vcs.savannah.gnu.org>
2015-01-16 19:10 ` [Emacs-diffs] master b689b90: Package archives now have priorities Glenn Morris
@ 2015-01-16 23:48 ` Artur Malabarba
2015-01-17 11:02 ` [Emacs-diffs] " Jorgen Schäfer
1 sibling, 1 reply; 15+ messages in thread
From: Artur Malabarba @ 2015-01-16 23:48 UTC (permalink / raw)
To: emacs-devel, Jorgen Schaefer; +Cc: emacs-diffs
> +(defcustom package-archive-priorities nil
> + "An alist of priorities for packages.
> +
> +Each element has the form (ARCHIVE-ID . PRIORITY).
> +
> +When installing packages, the package with the highest version
> +number from the archive with the highest priority is
> +selected. When higher versions are available from archives with
> +lower priorities, the user has to select those manually.
> +
> +Archives not in this list have the priority 0."
> + :type 'integer
I think you meant 'alist?
> +(defun package--add-to-alist (pkg-desc alist)
> + "Add PKG-DESC to ALIST.
> +
> +Packages are grouped by name. The package descriptions are sorted
> +by version number."
> + (let* ((name (package-desc-name pkg-desc))
> + (priority-version (package-desc-priority-version pkg-desc))
> + (existing-packages (assq name alist)))
> + (if (not existing-packages)
> + (cons (list name pkg-desc)
This list should be a cons, probably why the test is failing.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-16 23:48 ` Artur Malabarba
@ 2015-01-17 11:02 ` Jorgen Schäfer
2015-01-17 12:12 ` Artur Malabarba
0 siblings, 1 reply; 15+ messages in thread
From: Jorgen Schäfer @ 2015-01-17 11:02 UTC (permalink / raw)
To: bruce.connor.am; +Cc: emacs-devel
On Sat, Jan 17, 2015 at 12:48 AM, Artur Malabarba
<bruce.connor.am@gmail.com> wrote:
>> +(defcustom package-archive-priorities nil
>> + "An alist of priorities for packages.
>> +
>> +Each element has the form (ARCHIVE-ID . PRIORITY).
>> +
>> +When installing packages, the package with the highest version
>> +number from the archive with the highest priority is
>> +selected. When higher versions are available from archives with
>> +lower priorities, the user has to select those manually.
>> +
>> +Archives not in this list have the priority 0."
>> + :type 'integer
> I think you meant 'alist?
Indeed, thanks for catching this. Fixed in d80fed.
>> +(defun package--add-to-alist (pkg-desc alist)
>> + "Add PKG-DESC to ALIST.
>> +
>> +Packages are grouped by name. The package descriptions are sorted
>> +by version number."
>> + (let* ((name (package-desc-name pkg-desc))
>> + (priority-version (package-desc-priority-version pkg-desc))
>> + (existing-packages (assq name alist)))
>> + (if (not existing-packages)
>> + (cons (list name pkg-desc)
> This list should be a cons, probably why the test is failing.
Why should this be a cons? The alist maps package names to ordered
package descriptors – I guess (cons name (list pkg-desc)) would be
clearer in intent.
Regards,
Jorgen
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-17 11:02 ` [Emacs-diffs] " Jorgen Schäfer
@ 2015-01-17 12:12 ` Artur Malabarba
2015-01-17 12:27 ` Jorgen Schäfer
2015-01-17 14:11 ` Stefan Monnier
0 siblings, 2 replies; 15+ messages in thread
From: Artur Malabarba @ 2015-01-17 12:12 UTC (permalink / raw)
To: Jorgen Schäfer; +Cc: emacs-devel
>>> +(defun package--add-to-alist (pkg-desc alist)
>>> + "Add PKG-DESC to ALIST.
>>> +
>>> +Packages are grouped by name. The package descriptions are sorted
>>> +by version number."
>>> + (let* ((name (package-desc-name pkg-desc))
>>> + (priority-version (package-desc-priority-version pkg-desc))
>>> + (existing-packages (assq name alist)))
>>> + (if (not existing-packages)
>>> + (cons (list name pkg-desc)
>> This list should be a cons, probably why the test is failing.
>
> Why should this be a cons? The alist maps package names to ordered
> package descriptors – I guess (cons name (list pkg-desc)) would be
> clearer in intent.
Reading your code again, I see there are places where this `list' is
correct, but there's also one point where I think it's wrong.
Note the following diff section. It used to push `(cons
(package-desc-name pkg-desc) pkg-desc)', and now it uses
`package--add-to-alist' which pushes a list instead of a cons. Do you
see what I mean?
(defun package--download-one-archive (archive file)
"Retrieve an archive file FILE from ARCHIVE, and cache it.
ARCHIVE should be a cons cell of the form (NAME . LOCATION),
@@ -1991,18 +2035,18 @@ If optional arg BUTTON is non-nil, describe
its associated package."
;; ENTRY is (PKG-DESC [NAME VERSION STATUS DOC])
(let ((pkg-desc (car entry))
(status (aref (cadr entry) 2)))
- (cond ((member status '("installed" "unsigned"))
- (push pkg-desc installed))
- ((member status '("available" "new"))
- (push (cons (package-desc-name pkg-desc) pkg-desc)
- available)))))
+ (cond ((member status '("installed" "unsigned"))
+ (push pkg-desc installed))
+ ((member status '("available" "new"))
+ (setq available (package--add-to-alist pkg-desc available))))))
Maybe the solution is not to change `package--add-to-alist' but simply
to not use it here.
I also have a very tiny peeve here. Could we name this function
`package--alist-append' or something like this?
It's just that `add-to-alist' really reminds me of `add-to-list',
which has a very different effect (the list variable is the first
argument, it's referenced by name instead of value, and it's changed
destructively).
If you don't agree I won't push it. :-)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-17 12:12 ` Artur Malabarba
@ 2015-01-17 12:27 ` Jorgen Schäfer
2015-01-17 14:11 ` Stefan Monnier
1 sibling, 0 replies; 15+ messages in thread
From: Jorgen Schäfer @ 2015-01-17 12:27 UTC (permalink / raw)
To: Artur Malabarba; +Cc: emacs-devel
On Sat, Jan 17, 2015 at 1:12 PM, Artur Malabarba
<bruce.connor.am@gmail.com> wrote:
>>>> +(defun package--add-to-alist (pkg-desc alist)
>>>> + "Add PKG-DESC to ALIST.
>>>> +
>>>> +Packages are grouped by name. The package descriptions are sorted
>>>> +by version number."
>>>> + (let* ((name (package-desc-name pkg-desc))
>>>> + (priority-version (package-desc-priority-version pkg-desc))
>>>> + (existing-packages (assq name alist)))
>>>> + (if (not existing-packages)
>>>> + (cons (list name pkg-desc)
>>> This list should be a cons, probably why the test is failing.
>>
>> Why should this be a cons? The alist maps package names to ordered
>> package descriptors – I guess (cons name (list pkg-desc)) would be
>> clearer in intent.
>
> Reading your code again, I see there are places where this `list' is
> correct, but there's also one point where I think it's wrong.
> Note the following diff section. It used to push `(cons
> (package-desc-name pkg-desc) pkg-desc)', and now it uses
> `package--add-to-alist' which pushes a list instead of a cons. Do you
> see what I mean?
Yes – the code used to keep only one version around, which made
finding the correct version by archive priority more tricky, so I
changed it to keep a full list. That was also what another part of the
code already did, so I could unify the two into a single function. All
accesses to that list should be updated appropriately.
(The whole logic in package.el is quite convoluted and has a lot of
duplication with subtly different semantics, so I would not be too
surprised if there are some edge cases I missed, though.)
> I also have a very tiny peeve here. Could we name this function
> `package--alist-append' or something like this?
>
> It's just that `add-to-alist' really reminds me of `add-to-list',
> which has a very different effect (the list variable is the first
> argument, it's referenced by name instead of value, and it's changed
> destructively).
> If you don't agree I won't push it. :-)
Go ahead, I do not have any strong feelings about the names.
Regards,
Jorgen
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-17 12:12 ` Artur Malabarba
2015-01-17 12:27 ` Jorgen Schäfer
@ 2015-01-17 14:11 ` Stefan Monnier
2015-01-17 21:44 ` Stephen J. Turnbull
1 sibling, 1 reply; 15+ messages in thread
From: Stefan Monnier @ 2015-01-17 14:11 UTC (permalink / raw)
To: Artur Malabarba; +Cc: Jorgen Schäfer, emacs-devel
> I also have a very tiny peeve here. Could we name this function
> `package--alist-append' or something like this?
> It's just that `add-to-alist' really reminds me of `add-to-list',
> which has a very different effect (the list variable is the first
> argument, it's referenced by name instead of value, and it's changed
> destructively).
FWIW, I had the same reaction (and since I really dislike add-to-list's
use "call by symbol", I actully went to double check what
package--add-to-alist did, fearing it made the same design mistake).
Stefan
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-17 14:11 ` Stefan Monnier
@ 2015-01-17 21:44 ` Stephen J. Turnbull
0 siblings, 0 replies; 15+ messages in thread
From: Stephen J. Turnbull @ 2015-01-17 21:44 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Jorgen Schäfer, Artur Malabarba, emacs-devel
Stefan Monnier writes:
> FWIW, I had the same reaction (and since I really dislike
> add-to-list's use "call by symbol",
So fix it and call it "push-unique" or something like that. (Yes, I
know about the APPEND argument, but I wonder how often that is
computed -- if never, then "push" is the default.)
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-16 19:33 ` Jorgen Schäfer
2015-01-16 20:09 ` Glenn Morris
@ 2015-01-19 21:40 ` Glenn Morris
2015-01-20 9:03 ` Jorgen Schäfer
1 sibling, 1 reply; 15+ messages in thread
From: Glenn Morris @ 2015-01-19 21:40 UTC (permalink / raw)
To: Jorgen Schäfer; +Cc: emacs-devel
Jorgen Schäfer wrote:
> I hope 78e6ccc fixes the test
No, it still fails.
Can you not, err, test this?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-19 21:40 ` Glenn Morris
@ 2015-01-20 9:03 ` Jorgen Schäfer
2015-01-20 15:26 ` Dmitry Gutov
2015-01-21 20:06 ` Glenn Morris
0 siblings, 2 replies; 15+ messages in thread
From: Jorgen Schäfer @ 2015-01-20 9:03 UTC (permalink / raw)
To: Glenn Morris; +Cc: emacs-devel
On Mon, Jan 19, 2015 at 10:40 PM, Glenn Morris <rgm@gnu.org> wrote:
> Jorgen Schäfer wrote:
>
>> I hope 78e6ccc fixes the test
>
> No, it still fails.
Do you have a link to the hydra output so I can see what fails on
hydra? As I mentioned, hydra seems to run tests in a different
environment than I do on my own machine.
> Can you not, err, test this?
That is a tricky question.
Yes, I can test this. But no, I can't make the test pass.
All the package-test-install-* tests fail with an error "Autoloads
file [...] lacks boilerplate". This seems to be a problem in the test
suite code, and was the case before my commit. I was unable to figure
out how to fix it, so I did not.
As much as I could test my test code locally without the Emacs test
suite, it seemed to pass.
I was hoping the different test environment on hydra would fix the
autoloads problem. I do not know what the error now was on hydra, so I
can't say, but if it is the autoloads error, I do not know how to fix
it.
Still, I figured a test case that would most likely pass if the test
suite was not broken was better than no test case. Would it be better
if I removed my test case again?
Regards,
Jorgen
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-20 9:03 ` Jorgen Schäfer
@ 2015-01-20 15:26 ` Dmitry Gutov
2015-01-20 17:11 ` Jorgen Schäfer
2015-01-21 20:06 ` Glenn Morris
1 sibling, 1 reply; 15+ messages in thread
From: Dmitry Gutov @ 2015-01-20 15:26 UTC (permalink / raw)
To: Jorgen Schäfer, Glenn Morris; +Cc: emacs-devel
On 01/20/2015 11:03 AM, Jorgen Schäfer wrote:
> Yes, I can test this. But no, I can't make the test pass.
Do you run 'make check'? I have only two test failures on my local
machine: yours and vc-test-svn03-working-revision.
> All the package-test-install-* tests fail with an error "Autoloads
> file [...] lacks boilerplate". This seems to be a problem in the test
> suite code, and was the case before my commit. I was unable to figure
> out how to fix it, so I did not.
Why don't you file a bug about it?
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-20 15:26 ` Dmitry Gutov
@ 2015-01-20 17:11 ` Jorgen Schäfer
2015-01-20 17:19 ` Dmitry Gutov
0 siblings, 1 reply; 15+ messages in thread
From: Jorgen Schäfer @ 2015-01-20 17:11 UTC (permalink / raw)
To: Dmitry Gutov; +Cc: emacs-devel
On Tue, Jan 20, 2015 at 4:26 PM, Dmitry Gutov <dgutov@yandex.ru> wrote:
> On 01/20/2015 11:03 AM, Jorgen Schäfer wrote:
>
>> Yes, I can test this. But no, I can't make the test pass.
>
> Do you run 'make check'? I have only two test failures on my local machine:
> yours and vc-test-svn03-working-revision.
Yes.
As I mentioned, make -C test/automated gives me 46 test failures.
>> All the package-test-install-* tests fail with an error "Autoloads
>> file [...] lacks boilerplate". This seems to be a problem in the test
>> suite code, and was the case before my commit. I was unable to figure
>> out how to fix it, so I did not.
>
> Why don't you file a bug about it?
I'm not even sure if it is a bug, or if I am doing something wrong. I
do not have the time to investigate this in any detail.
As this is indeed my test case and not this weird autoload problem I
see, and I simply lack the time to work on this, I have removed my
test case. Apologies to everyone for the noise.
Regards,
Jorgen
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-20 17:11 ` Jorgen Schäfer
@ 2015-01-20 17:19 ` Dmitry Gutov
0 siblings, 0 replies; 15+ messages in thread
From: Dmitry Gutov @ 2015-01-20 17:19 UTC (permalink / raw)
To: Jorgen Schäfer; +Cc: emacs-devel
On 01/20/2015 07:11 PM, Jorgen Schäfer wrote:
>> Why don't you file a bug about it?
>
> I'm not even sure if it is a bug, or if I am doing something wrong.
So what? People file invalid bugs all the time.
> As this is indeed my test case and not this weird autoload problem I
> see, and I simply lack the time to work on this, I have removed my
> test case.
You've kept the now-untested feature, though.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Emacs-diffs] master b689b90: Package archives now have priorities.
2015-01-20 9:03 ` Jorgen Schäfer
2015-01-20 15:26 ` Dmitry Gutov
@ 2015-01-21 20:06 ` Glenn Morris
1 sibling, 0 replies; 15+ messages in thread
From: Glenn Morris @ 2015-01-21 20:06 UTC (permalink / raw)
To: Jorgen Schäfer; +Cc: emacs-devel
Jorgen Schäfer wrote:
> All the package-test-install-* tests fail with an error "Autoloads
> file [...] lacks boilerplate".
It seems like your build might be broken.
Personally I would do a `make boostrap'.
> I was hoping the different test environment on hydra would fix the
> autoloads problem. I do not know what the error now was on hydra, so I
> can't say, but if it is the autoloads error, I do not know how to fix
> it.
http://hydra.nixos.org/build/18990023
http://hydra.nixos.org/build/18990023/log/raw
Test package-test-install-prioritized condition:
(ert-test-failed
((should
(version-list-= '...
(package-desc-version installed)))
:form
(version-list-=
(1 3)
(1 4))
:value nil))
FAILED 7/16 package-test-install-prioritized
This is exactly the error I was getting on RHEL7, which is quite a
different system to hydra. It seems unlikely that something like this
would be platform specific. IIUC, it indicates the priority mechanism
simply isn't working.
> Still, I figured a test case that would most likely pass if the test
> suite was not broken was better than no test case.
AFAIK, the test suite is not broken.
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-01-21 20:06 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20150116102411.11014.8945@vcs.savannah.gnu.org>
[not found] ` <E1YC44Z-0002sG-4a@vcs.savannah.gnu.org>
2015-01-16 19:10 ` [Emacs-diffs] master b689b90: Package archives now have priorities Glenn Morris
2015-01-16 19:33 ` Jorgen Schäfer
2015-01-16 20:09 ` Glenn Morris
2015-01-19 21:40 ` Glenn Morris
2015-01-20 9:03 ` Jorgen Schäfer
2015-01-20 15:26 ` Dmitry Gutov
2015-01-20 17:11 ` Jorgen Schäfer
2015-01-20 17:19 ` Dmitry Gutov
2015-01-21 20:06 ` Glenn Morris
2015-01-16 23:48 ` Artur Malabarba
2015-01-17 11:02 ` [Emacs-diffs] " Jorgen Schäfer
2015-01-17 12:12 ` Artur Malabarba
2015-01-17 12:27 ` Jorgen Schäfer
2015-01-17 14:11 ` Stefan Monnier
2015-01-17 21:44 ` Stephen J. Turnbull
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).