Fix some TravisCI issues #2059

xiongtx · 2017-07-22T04:55:26Z

@bbatsov originally raised this issue when discussing #2057.

bbatsov · 2017-07-22T04:59:07Z

test/cider-checks.el

@@ -20,5 +20,9 @@
                              "\\`[^.].*\\.el\\'" t)))
  (dolist (file files)
    (checkdoc-file file))
+  (when (get-buffer "*Warnings*")
+    (message "Failing due to checkdoc warnings...")


I think we should dump the contents of the warnings buffer as well, otherwise it would be really hard for people to figure out what exactly is wrong.

Warnings are already dumped line-by-line.

Ah, OK. Thought they were only in the warnings buffer.

bbatsov · 2017-07-22T05:04:25Z

.travis.yml

  - EMACS_BINARY=emacs-25.1-travis MAKE_TEST=test
+  - EMACS_BINARY=emacs-25.1-travis MAKE_TEST=test-bytecomp
+  - EMACS_BINARY=emacs-25.1-travis MAKE_TEST=test-checks


We should add 25.2 here as well, as it was recently released. Probably in a couple of releases we should drop support for Emacs 24 completely, but it seems too early now.

bbatsov · 2017-07-22T05:05:35Z

test/cider-checks.el

@@ -6,6 +6,9 @@
 ;; This assumes that all CIDER dependencies are already on the package dir
 ;; (probably from running `cask install').

+;; NOTE: `checkdoc-file' is introduced in Emacs 25, so do not run this check with
+;; Emacs 24.x


I guess you can also add a version check around the code itself.

`directory-files-recursively` is [introduced in Emacs 25](https://github.com/purcell/package-lint/blob/4e4b34fc4f12ef2f7965fa959c5809aacdb6af63/package-lint.el#L186).

xiongtx · 2017-07-22T05:33:24Z

Most tests are passing, but there's an outstanding issue with byte-compilation tests for Emacs 24.x.

bbatsov · 2017-07-22T06:18:41Z

Most tests are passing, but there's an outstanding issue with byte-compilation tests for Emacs 24.x.

Yeah, probably this is a bug, or something changed in Emacs 25. I guess you can simply replace cl-assert with some check + error. Seems like the simple solution to this problem.

Doing so causes a [byte-compiler error](https://travis-ci.org/clojure-emacs/cider/jobs/256287467#L1214) under Emacs 24. See: https://emacs.stackexchange.com/questions/34341/error-called-with-1-args-to-fill-0-format-fields/34350#34350

xiongtx · 2017-07-22T21:20:39Z

R.e. the cl-assert issue, it's actually a bug in CIDER. See explanation.

`'nil` is fine in Emacs 25, but not in Emacs 24.

xiongtx · 2017-07-22T21:39:32Z

@bbatsov All tests pass.

bbatsov · 2017-07-23T05:28:08Z

R.e. the cl-assert issue, it's actually a bug in CIDER. See explanation.

Quite interesting.

Thanks for tackling all of this! Great work!

bbatsov · 2017-07-23T05:53:47Z

P.S. It would be great if you can do all of this for clojure-mode as well.

vspinu · 2017-07-23T23:36:58Z

@bbatsov would you be so kind and give some more consideration to the parameter order and double space "conventions"?

Double space is unnatural and never used in other contexts. So you are bound to forget those and run into make test issues. Each time you forget about it - go and figure location, fix, re-commit, re-check etc. Good 5-10 minutes of wasted time with zero benefit. Plus flow disruption.

Same goes with order of arguments, wasting precious minutes of how to twist untwistable and forcing users to read pseudo-english is rather silly.

As to the "conventions" argument, no-one seems to care about that in the Emacs proper. Pretty much every core file I tried has checkdoc issues much more serious than double spaces and order of arguments.

Conventions which don't benefit the user or the programmer are silly and deserve to be disregarded anyways. These two are clearly of that sort.

xiongtx · 2017-07-24T00:30:01Z

The order of arguments check is already turned off.

The two-space rule is an Emacs convention that should be followed. We could turn it off by setting sentence-end-double-space to nil while running checkdoc, but I don't think we should.

vspinu · 2017-07-24T08:57:40Z

should be followed

Why is that exactly? Even auto-fill doesn't respect it, why should you?

Double space adds more inefficiencies because it happens more often. So, if you already disabled order of args you should consider disabling double space.

bbatsov · 2017-07-24T09:32:26Z

Double space adds more inefficiencies because it happens more often. So, if you already disabled order of args you should consider disabling double space.

We've maintained the codebase consistent in this regard since day 1 of the project and I see little point to change this now. It's not a big effort and you simply get used to this after a while. Most Elisp projects stick to this convention so it stopped bothering me after a while. I'm also using flycheck, so I get immediate visual feedback if something is somewhere there's a missing space. (and I think many Elisp hackers would benefit from adding it)

Double space is unnatural and never used in other contexts. So you are bound to forget those and run into make test issues. Each time you forget about it - go and figure location, fix, re-commit, re-check etc. Good 5-10 minutes of wasted time with zero benefit. Plus flow disruption.

I certainly don't want to inconvenience anyone, but I'm big on consistency in the codebase of a project. Can't really go back on this given how much code was written following this style - "fixing" this would create a diff that would obscure half the codebase and would create inconsistency with all of my other projects, that I don't want to introduce.

bbatsov · 2017-07-24T09:52:56Z

Why is that exactly? Even auto-fill doesn't respect it, why should you?

auto-fill doesn't change spacing around punctuation, so I don't see how this is relevant.

vspinu · 2017-07-24T15:09:09Z

I'm also using flycheck, so I get immediate visual feedback if something is somewhere there's a missing space.

I was never happy with flycheck for emacs. Most of the libraries have checkdoc issues and that would mean constantly seeing those annoying highlights. Cider could be honorable exception where I can keep it active though, but ... it doesn't work in Cider project. I am getting

Suspicious state from syntax checker emacs-lisp-checkdoc: Flycheck checker emacs-lisp-checkdoc returned non-zero exit code 255, but its output contained no errors: Invalid function: "top-level"

All other projects are fine. Any ideas?

Can't really go back on this given how much code was written following this style

Fair enough.

auto-fill doesn't change spacing around punctuation,

It used to be the case in previous emacses, when you could end up with a sentence starting a new para. But I think that was fixed in 25.x as I haven't encountered the issue for a while.

vspinu · 2017-08-02T19:31:35Z

I am unable to run test-bytecomp and test-checks on my local machine with emacs 25.2 and 26. I am getting cryptic stuff like:

cider$ make test-checks
emacs-25.2 --version
GNU Emacs 25.2.2
Copyright (C) 2017 Free Software Foundation, Inc.
GNU Emacs comes with ABSOLUTELY NO WARRANTY.
You may redistribute copies of GNU Emacs
under the terms of the GNU General Public License.
For more information about these matters, see the file named COPYING.
cask exec emacs-25.2 -q --no-site-file --no-site-lisp --batch \
	-l test/cider-checks.el ./
Autoloading failed to define function checkdoc-file
Makefile:31: recipe for target 'test-checks' failed
make: *** [test-checks] Error 255

and

cider$ make test-bytecomp
Compute dependencies
emacs25 --version
GNU Emacs 25.2.2
Copyright (C) 2017 Free Software Foundation, Inc.
GNU Emacs comes with ABSOLUTELY NO WARRANTY.
You may redistribute copies of GNU Emacs
under the terms of the GNU General Public License.
For more information about these matters, see the file named COPYING.
cask exec emacs25 --no-site-file --no-site-lisp --batch \
	-l test/cider-bytecomp-warnings.el cider-test.el
Making version-control local to cider-autoloads.el while let-bound!
Loading /home/vspinu/VC/cider/cider-autoloads.el (source)...

In toplevel form:
cider-test.el:736:1:Error: the function `seq-filter' is not known to be defined.
Makefile:37: recipe for target 'cider-test.elc-test' failed
make: *** [cider-test.elc-test] Error 1

Any pointers on this?

bbatsov · 2017-08-03T03:51:25Z

The second failure looks like some missing require in the tests - (require 'seq), but the first puzzles me. Seems for some reason check-file is missing, but it's a built-in, so I can't imagine how this can happen.

xiongtx · 2017-08-03T04:14:41Z

Cannot reproduce on Emacs 25.1.1. Have you run make elpa beforehand?

vspinu · 2017-08-03T10:52:19Z

Have you run make elpa beforehand?

(BTW, make clean doesn't clean elpa. By design?)

Yes, but cask doesn't pick the EMACS variable. With emacs25 I am still seeing only 24.5.1 subdir

Wrote /home/vspinu/VC/cider/.cask/24.5.1/elpa/spinner-1.7.3/spinner-autoloads.el
Wrote /home/vspinu/VC/cider/.cask/24.5.1/elpa/seq-2.20/seq-autoloads.el
Wrote /home/vspinu/VC/cider/.cask/24.5.1/elpa/clojure-mode-20170725.2307/clojure-mode-autoloads.el
Wrote /home/vspinu/VC/cider/.cask/24.5.1/elpa/buttercup-20170729.609/buttercup-autoloads.el

I thought the problem was that seq there. If I remove it manually and run with emacs25 I still get missing seq

emacs25 --version
GNU Emacs 25.2.2
....
In toplevel form:
cider-test.el:31:1:Error: Cannot open load file: No such file or directory, seq
Makefile:37: recipe for target 'cider-test.elc-test' failed
make: *** [cider-test.elc-test] Error 1

It turned out to be the load path. For some unclear reason when I ran make test-bytecomp with emacs 25 or 26 I am still getting emacs 24 load path:

"Load-path: (/home/vspinu/VC/cider/utils/ /home/vspinu/VC/cider/ /home/vspinu/VC/cider/.cask/24.5.1/elpa/spinner-1.7.3 /home/vspinu/VC/cider/.cask/24.5.1/elpa/queue-0.1.1 /home/vspinu/VC/cider/.cask/24.5.1/elpa/pkg-info-20150517.443 /home/vspinu/VC/cider/.cask/24.5.1/elpa/epl-20150517.433 /home/vspinu/VC/cider/.cask/24.5.1/elpa/clojure-mode-20170725.2307 /home/vspinu/VC/cider/.cask/24.5.1/elpa/markdown-mode-20170802.646 /home/vspinu/VC/cider/.cask/24.5.1/elpa/buttercup-20170729.609
 /usr/share/emacs/24.5/lisp /usr/share/emacs/24.5/lisp/vc /usr/share/emacs/24.5/lisp/url /usr/share/emacs/24.5/lisp/textmodes /usr/share/emacs/24.5/lisp/progmodes /usr/share/emacs/24.5/lisp/play /usr/share/emacs/24.5/lisp/org /usr/share/emacs/24.5/lisp/nxml /usr/share/emacs/24.5/lisp/net /usr/share/emacs/24.5/lisp/mh-e /usr/share/emacs/24.5/lisp/mail /usr/share/emacs/24.5/lisp/leim /usr/share/emacs/24.5/lisp/language
 /usr/share/emacs/24.5/lisp/international /usr/share/emacs/24.5/lisp/gnus /usr/share/emacs/24.5/lisp/eshell /usr/share/emacs/24.5/lisp/erc /usr/share/emacs/24.5/lisp/emulation /usr/share/emacs/24.5/lisp/emacs-lisp /usr/share/emacs/24.5/lisp/cedet /usr/share/emacs/24.5/lisp/calendar /usr/share/emacs/24.5/lisp/calc /usr/share/emacs/24.5/lisp/obsolete /usr/share/emacs/24.5/lisp/vc /usr/share/emacs/24.5/lisp/url /usr/share/emacs/24.5/lisp/textmodes /usr/share/emacs/24.5/lisp/progmodes /usr/share/emacs/24.5/lisp/play /usr/share/emacs/24.5/lisp/org /usr/share/emacs/24.5/lisp/nxml /usr/share/emacs/24.5/lisp/net /usr/share/emacs

which is bizarre :(

vspinu · 2017-08-03T11:28:22Z

It's on the cask's side (cask/cask#335, cask/cask#295, cask/cask#260 seem to be relevant). I am not following all of that but export EMACS = emacs25 does seem to work, while export CASK_EMACS = emacs25 doesn't.

bbatsov reviewed Jul 22, 2017

View reviewed changes

xiongtx force-pushed the travis-testing branch from 2617617 to 9059be2 Compare July 22, 2017 05:04

bbatsov reviewed Jul 22, 2017

View reviewed changes

Tianxiang Xiong and others added 6 commits July 21, 2017 22:16

Add more Travis test combinations

1f4da29

Fix misplaced form in cider-browse-spec--pprint

6f479ed

Add directory-files-recursively to cider-compat

7ce96b5

`directory-files-recursively` is [introduced in Emacs 25](https://github.com/purcell/package-lint/blob/4e4b34fc4f12ef2f7965fa959c5809aacdb6af63/package-lint.el#L186).

Fail on checkdoc warnings

c39d74f

Fix checkdoc errors

9555fbc

Check that checkdoc-file is defined before running checks

2a68e29

xiongtx force-pushed the travis-testing branch from 9059be2 to 2a68e29 Compare July 22, 2017 05:17

Don't set show-args to t in cl-assert

81dad9d

Doing so causes a [byte-compiler error](https://travis-ci.org/clojure-emacs/cider/jobs/256287467#L1214) under Emacs 24. See: https://emacs.stackexchange.com/questions/34341/error-called-with-1-args-to-fill-0-format-fields/34350#34350

xiongtx added 2 commits July 22, 2017 14:34

Make reference to cider-stacktrace-toggle-all a function

d25c51b

Use nil value instead of symbol in pcase

5b25c69

`'nil` is fine in Emacs 25, but not in Emacs 24.

xiongtx force-pushed the travis-testing branch from a8a1c95 to 5b25c69 Compare July 22, 2017 21:34

bbatsov merged commit ba66d22 into clojure-emacs:master Jul 23, 2017

xiongtx deleted the travis-testing branch July 23, 2017 07:59

xiongtx mentioned this pull request Jul 23, 2017

Use evm for TravisCI testing clojure-emacs/clojure-mode#440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some TravisCI issues #2059

Fix some TravisCI issues #2059

xiongtx commented Jul 22, 2017

bbatsov Jul 22, 2017

xiongtx Jul 22, 2017

bbatsov Jul 22, 2017

bbatsov Jul 22, 2017

xiongtx Jul 22, 2017

bbatsov Jul 22, 2017

xiongtx Jul 22, 2017

xiongtx commented Jul 22, 2017

bbatsov commented Jul 22, 2017

xiongtx commented Jul 22, 2017

xiongtx commented Jul 22, 2017

bbatsov commented Jul 23, 2017

bbatsov commented Jul 23, 2017

vspinu commented Jul 23, 2017 •

edited

Loading

xiongtx commented Jul 24, 2017

vspinu commented Jul 24, 2017 •

edited

Loading

bbatsov commented Jul 24, 2017

bbatsov commented Jul 24, 2017

vspinu commented Jul 24, 2017

vspinu commented Aug 2, 2017

bbatsov commented Aug 3, 2017

xiongtx commented Aug 3, 2017

vspinu commented Aug 3, 2017 •

edited

Loading

vspinu commented Aug 3, 2017

Fix some TravisCI issues #2059

Fix some TravisCI issues #2059

Conversation

xiongtx commented Jul 22, 2017

bbatsov Jul 22, 2017

Choose a reason for hiding this comment

xiongtx Jul 22, 2017

Choose a reason for hiding this comment

bbatsov Jul 22, 2017

Choose a reason for hiding this comment

bbatsov Jul 22, 2017

Choose a reason for hiding this comment

xiongtx Jul 22, 2017

Choose a reason for hiding this comment

bbatsov Jul 22, 2017

Choose a reason for hiding this comment

xiongtx Jul 22, 2017

Choose a reason for hiding this comment

xiongtx commented Jul 22, 2017

bbatsov commented Jul 22, 2017

xiongtx commented Jul 22, 2017

xiongtx commented Jul 22, 2017

bbatsov commented Jul 23, 2017

bbatsov commented Jul 23, 2017

vspinu commented Jul 23, 2017 • edited Loading

xiongtx commented Jul 24, 2017

vspinu commented Jul 24, 2017 • edited Loading

bbatsov commented Jul 24, 2017

bbatsov commented Jul 24, 2017

vspinu commented Jul 24, 2017

vspinu commented Aug 2, 2017

bbatsov commented Aug 3, 2017

xiongtx commented Aug 3, 2017

vspinu commented Aug 3, 2017 • edited Loading

vspinu commented Aug 3, 2017

vspinu commented Jul 23, 2017 •

edited

Loading

vspinu commented Jul 24, 2017 •

edited

Loading

vspinu commented Aug 3, 2017 •

edited

Loading