Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Glenn Morris]

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.

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Jorgen Schäfer]

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

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Glenn Morris]

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

Re: master b689b90: Package archives now have priorities.

[Artur Malabarba]

> +(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.

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Jorgen Schäfer]

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

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Artur Malabarba]

>>> +(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. :-)

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Jorgen Schäfer]

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

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Stefan Monnier]

> 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

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Stephen J. Turnbull]

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.)

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Glenn Morris]

Jorgen Schäfer wrote:

> I hope 78e6ccc fixes the test

No, it still fails.
Can you not, err, test this?

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Jorgen Schäfer]

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

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Dmitry Gutov]

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?

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Jorgen Schäfer]

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

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Dmitry Gutov]

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.

Re: [Emacs-diffs] master b689b90: Package archives now have priorities.

[Glenn Morris]

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.