-
Notifications
You must be signed in to change notification settings - Fork 79
Adopt makem.sh for build support (linting, running tests) #432
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
base: master
Are you sure you want to change the base?
Conversation
Discovered some issues on macos, opening alphapapa/makem.sh#49 |
8b57f20
to
1d05017
Compare
Updated makem.sh to include the macOS fix. |
I'd be generally in favour of this, because I'm not sure if any of the more active maintainers have any specific love for cmake, and it would be good to include more linting etc. in the CI. |
Also use makem.sh's support for checkdoc, since the previously-used checkdoc-batch URL is broken.
Currently, we need the --compile-batch option because compiling the files individually fails on Emacs 25 (I think due to subr-x macros not being loaded at startup).
We just sprinkle some eval-when-compiles around for older Emacs. We can remove them when the CI no longer supports Emacs 25.1.
Isn't this just replacing one big blob (well a shell script isn't technically a blob but this one is very big) with another? If this adapted I would recommend a submodule instead copying the file into the repository to make it easier to track the script. |
I'm a big Makefile fan, but the setup here is pretty overbuilt for current needs, misses common elisp build steps like linting, and none of the maintainers here are likely to want to add it. Meanwhile makem is fairly well used and actively supported, which is why I'm broadly in favour of the change. No other maintainers have expressed a preference either way.
That would be fine too, though I don't know if the script has to end up in the root of the repo to work properly. |
Steve Purcell ***@***.***> writes:
purcell left a comment (ledger/ledger-mode#432)
> Isn't this just replacing one big blob (well a shell script isn't
> technically a blob but this one is very big) with another? Compared
> to that a Makefile is much shorter.
I'm a big Makefile fan, but the setup here is pretty overbuilt for
current needs, misses common elisp build steps like linting, and none
of the maintainers here are likely to want to add it. Meanwhile makem
is fairly well used and actively supported, which is why I'm broadly
in favour of the change. No other maintainers have expressed a
preference either way.
Personally I hope that the classical
make
make install
still works.
CMake can handle test cases just fine but I get why it might not be the
right tool when there's no direct CMake Elisp package implementing all
the features.
> If this adapted I would recommend a submodule instead copying the file into the repository to make it easier to track the script.
That would be fine too, though I don't know if the script has to end up in the root of the repo to work properly.
A symlink could work.
|
Seems very reasonable! |
This PR switches from CMake and custom make rules to using
makem.sh, as suggested in #423.
It also includes a small tweak to just disable#431 was merged, so that change was removed upon rebase.ledger-mode/test-001
(whichrequires running Emacs interactively) when makem.sh runs tests, since it does
not provide a custom ERT selector. That change can be removed once #431 is
merged.
Fix #423.