Fix compile errors - Githubissues

vermiculus commented 5 years ago

Going file-by-file, but I need to sign off for the night. See also #129 -- that will have to be addressed either with or before the merge.

vermiculus commented 5 years ago

I'm also unsure if compile-checking can reliably be added to the test suite with Cask. It wasn't straightforward when I created EMake (it's one of the reasons I created EMake in the first place). Would you be opposed to seeing what running the test suite with EMake instead of Cask would look like? I do know you're a fan of Cask already :wink:

See also vermiculus/emake.el#27.

Fuco1 commented 5 years ago

As far as running tests in CI goes I don't really care what is used. If you can prepare a PR (and if it's not too much work) I'll be happy to at least see how it's done.

If it can run buttercup and if I can also use the same stack locally that would be nice.

vermiculus commented 5 years ago

To explain the above 🙃 the tests have been 'working' (buttercup tests are failing – maybe from the compile? – but they're running) for me locally for some time. I couldn't figure out why Travis was having a hard time with it, so I added some debug statements. Turns out this line of the following block

emake: determining package descriptor...
emake: looking for *-pkg.el file...
emake: /home/travis/build/emacs-elsa/Elsa/elsa.el
emake: /home/travis/build/emacs-elsa/Elsa/
emake: Elsa-pkg.el                                            <---
emake: looking for *-pkg.el file...done
emake: didn't find a package descriptor
emake: parsing headers in "elsa.el"...
emake: parsing headers in "elsa.el"...done
emake: determining package descriptor...done
Package lacks a "Version" or "Package-Version" header
make: *** [.emake/elpa] Error 255

indicates the problem. Travis is pulling to directory emacs-elsa/Elsa/, package.el is interpreting that to mean Elsa is the name of the package, so it looks for Elsa-pkg.el. This does not exist – the file's actual name is elsa-pkg.el (because the package's main file is elsa.el. Took so long to figure this out – made me laugh 😄

This will probably have to be fixed at the package.el level – it's odd that it's dependent on the name of the directory rather than the name of the main package file (though I suppose there's the argument of 'who knows where the main file is'… but that's my problem, not necessarily yours).

vermiculus commented 5 years ago

I think this is ready for review. Some notes:

After I figured out the weird directory-casing package.el bug I noted above, most of the subsequent force-pushing was trying to figure out another therein bug -- the one behind https://github.com/vermiculus/emake.el/commit/e818fefd43e5c6f450f14a581f733b74fbcd7241. Oddly, I wasn't able to reproduce locally nor was I able to successfully do any poor-man's debugging with print statements and the like (to view in Travis's log files). I never actually figured it out; I ultimately decided to re-implement the small portion of package.el I needed rather than to continue a five-minute-per-cycle iterative debugging session.
The tests are failing -- and I believe they should be. It appears that after compiling on Emacs 26.1, some features of Elsa might indeed not work. I have not verified this by testing Elsa directly, though. Given that this is repeatable with the current testing command after a cask build, I don't think it's related to the use of EMake. Of course it is possible that it's related to the actual changes I made, but given those changes, I find it unlikely to cause such errors. It's also strange that it only happens to the compiled version.
Some warnings remain. Most are of the form discussed at #129, but there are a few others as of tonight (this morning?): one two three

To explore EMake as it's released by default (i.e., the Makefile that drives the Elisp), type make help. To try out what's relevant to this PR, make test and make compile are all you'll need. You should also make sure to set EMACS_VERSION on your development machine to whatever version (a la .travis.yml) that emacs (or rather, $(EMACS)) points to.

Fuco1 commented 5 years ago

Thanks Sean, looks really nice. I'll try to get a review in asap!

vermiculus commented 5 years ago

Related: #111

vermiculus commented 5 years ago

I'm 100% the pot calling the kettle black, but do you know when I should expect some feedback / ping you again? :-)

Fuco1 commented 5 years ago

I've been really bad with my github workflows lately :( Basically please be as annoying as you feel comfortable, I very much appreciate it. I'm now in a bad situation trying to juggle three big projects (work-related) but I still want to move things forward in OSS space as well.

I'm going to schedule this for today or tomorrow. I actually put a whole 4 hour block for Elsa every Sunday but... I didn't get to it this week :D

Fuco1 commented 5 years ago

@vermiculus How does emake change emacs versions? It seems to be very fast for the "released" versions but snapshot actually hangs. I've rebased this branch on top of current master and all tests pass except the snapshot. (#146)

Can you try to look at it? I think we're very close to accepting this patch.

vermiculus commented 5 years ago

Snapshot builds take longer – they can't be cached for obvious reasons and it appears releases are tar'd with some altered build settings. I have seen emacs-travis.mk choke on this sometimes (gnutls) and I'm not sure why, but updating emake usage may resolve the issue. I recently developed an alternative (dumbed-down, honestly) way to build emacs on CI.

Which PR should I push that commit to? #146?

vermiculus commented 5 years ago

Oh I see; #146 is just a rebased version of this PR 😄 I'll push it there.

vermiculus commented 5 years ago

Closing in favor of #146.

emacs-elsa / Elsa

Fix compile errors #130