Skip to content
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

Makefile dependencies and byte-compilation #15

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

medranocalvo
Copy link
Contributor

I'm just posting the makefile I use to see byte-compilation warnings. Is this

  • The dependencies allow parallel making (make -jN).
  • I like the byte-compilation warnings on xelb-gen. This implies creating the .elc. (Maybe that's not necessary?)

This is meant to be applied after #4.

@medranocalvo medranocalvo marked this pull request as draft January 18, 2024 10:46
@Stebalien
Copy link
Contributor

No strong objections, but does elisp-flymake-byte-compile not work for you? Or is this for CI?

@medranocalvo
Copy link
Contributor Author

I do use it (I actually use an uncommitted Makefile in EXWM as well). I've never triedelisp-flymake-byte-compile, will try to incorporate it.

We can also remove the byte-compilation rules altogether.

I'm undecided on whether lint on CI would be helpful or noisy (we all lint). Now that we have tests it might be worth it.

@medranocalvo medranocalvo force-pushed the medranocalvo/makefile-deps branch from 7208179 to 7ef1aa4 Compare January 19, 2024 10:10
@medranocalvo medranocalvo marked this pull request as ready for review January 19, 2024 10:10
@medranocalvo
Copy link
Contributor Author

We can also remove the byte-compilation rules altogether.

To clarify, I don't mean this sarcastically.

@Stebalien
Copy link
Contributor

We can also remove the byte-compilation rules altogether.

Remove them from where?

I'm undecided on whether lint on CI would be helpful or noisy (we all lint). Now that we have tests it might be worth it.

IMO, CI lints are nice because:

  1. Sometimes local linting is broken for some reason and, for tiny changes, you may not even notice it.
  2. Sometimes you make a tiny little fix that absolutely shouldn't break anything but does.
  3. It's a simple way to have a first-pass review for contributors.

Not helpful till we get the warnings down to 0, but helpful to keep it there.

@Stebalien
Copy link
Contributor

(note: I have no objections to this PR if you want to merge it, I'm just pointing out the alternatives)

@medranocalvo
Copy link
Contributor Author

We can also remove the byte-compilation rules altogether.

Remove them from where?

I meant removing them from the Makefile. But it will be useful for CI (see below).

I'm undecided on whether lint on CI would be helpful or noisy (we all lint). Now that we have tests it might be worth it.

IMO, CI lints are nice because:

1. Sometimes local linting is broken for some reason and, for tiny changes, you may not even notice it.

2. Sometimes you make a tiny little fix that absolutely shouldn't break anything but does.

Oh, it's not just me...

3. It's a simple way to have a first-pass review for contributors.

Not helpful till we get the warnings down to 0, but helpful to keep it there.

I agree on everything. I'm hopeful on the remaining warnings.


I'll try to automatize the discovery of Makefile dependencies if I find some time, to make this more robust. Otherwise I'll merge this.

@minad
Copy link
Member

minad commented Jan 19, 2024

Byte compilation is a good addition to the Makefile. We can also reuse this for CI, where we can compile on various Emacs versions via Steve Purcell's setup-emacs. I am in favor of adding linting, e.g., package-lint. however some linters like Melpazoid (#9) go a bit too far imo.

I'll try to automatize the discovery of Makefile dependencies if I find some time, to make this more robust. Otherwise I'll merge this.

Auto generating the dependencies would be great. Thanks!

* Makefile (generate): New phony goal for generating elisp sources
from XML protocol descriptions.
(compile): New phony goal to byte-compile all elisp.
(all): Depend on both.

* Makefile (ELGDS): Infer Makefile dependencies for generating.
(ELLDS): Infer Makefile dependencies for byte-compiling.

* xelb-gen (xelb-parse): Set `load-prefer-newer' to avoid picking
outdated byte-compiled definitions while generating.
@medranocalvo medranocalvo force-pushed the medranocalvo/makefile-deps branch from 7ef1aa4 to 6251e85 Compare January 31, 2024 12:16
@medranocalvo
Copy link
Contributor Author

I'll try to automatize the discovery of Makefile dependencies if I find some time, to make this more robust. Otherwise I'll merge this.

Auto generating the dependencies would be great. Thanks!

Now done. Works on my machine, please test.

It is common to hide the dependency Makefile fragments (.el.d in this patch) as dot-files or in a dot-folder. We can do that if you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants