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

counsel.el (counsel-compile): clean-up get-make-targets #1972

Closed

Conversation

@stsquad
Copy link
Contributor

@stsquad stsquad commented Mar 19, 2019

The original code called a pipeline of shell functions to get the list
of PHONY targets. One of the consequence of this is we picked up a
bunch of noise if the make crapped out with an error. To avoid this we
bring all the text extraction into emacs with a temp buffer and only
process it if make is happy.

This is also a first step towards what we would need if we wanted to
use counsel--async-command to gather stuff and update the targets
dynamically.

Copy link
Collaborator

@basil-conto basil-conto left a comment

I wonder if the built-in makefile-mode has any utility functions we can reuse for this purpose, instead of rolling out our own?

counsel.el Outdated

(defun counsel-compile-get-make-invocation (&optional blddir)
"Have a look in the root directory for any build control files.
The optional BLDDIR is useful for other helpers that have found
subdirectories that builds may be invoked in."
sub-directories that builds may be invoked in."
Copy link
Collaborator

@basil-conto basil-conto Mar 19, 2019

No need for this; contraction of commonly hyphenated pairs of words is perfectly normal, and a lot of Emacs documentation uses "subdirectories".

Copy link
Contributor Author

@stsquad stsquad Mar 20, 2019

fair enough - my spell checker overlay was being distracting so I fixed them but it's extra noise I'll remove...

(defun counsel--compile-get-make-targets (srcdir &optional blddir)
"Return a list of Make targets for a given SRCDIR/BLDDIR combination.
We search the Makefile for a list of phony targets which are
generally the top-level targets a Make system provides.
generally the top level targets a Make system provides.
Copy link
Collaborator

@basil-conto basil-conto Mar 19, 2019

I think "top-level" is more accurate here, since it's being used as an adjective.

counsel.el Outdated
The resulting strings are tagged with properties that
`counsel-compile-history' can use for filtering results."
(let* ((default-directory (or blddir srcdir))
(fmt (format (propertize "make %s %%s" 'cmd t)
(let* ((fmt (format (propertize "make %s %%s" 'cmd t)
Copy link
Collaborator

@basil-conto basil-conto Mar 19, 2019

This can become let instead of let* now, right?

counsel.el Outdated
@@ -5186,16 +5186,37 @@ N in your system."
"List of potential build subdirectory names to check for."
:type '(repeat directory))

(defvar counsel-compile-phony-pattern
(rx (: bol ".PHONY:" space (group (one-or-more print)) eol))
"Regexp for extracting PHONY targets from Makefiles.")
Copy link
Collaborator

@basil-conto basil-conto Mar 19, 2019

Nitpick: The terse form of rx is usually preferred, i.e.

(rx bol ".PHONY:" space (group (+ print)) eol)

I also suggest you write either "phony targets", as the GNU Make manual refers to them, or ".PHONY targets" (with the leading full stop).

counsel.el Outdated
(defun counsel-compile--probe-make-targets (dir)
"Return a list of Make targets for DIR.
If the make errors we return an empty list. This is usually because
Copy link
Collaborator

@basil-conto basil-conto Mar 19, 2019

"Return the empty list if Make exits with an error. This might happen because..."

counsel.el Outdated Show resolved Hide resolved
(targets))
(with-temp-buffer
;; 0 = no-rebuild, -q & 1 needs rebuild, 2 error
(when (> 2 (call-process "make" nil t nil "-nqp"))
Copy link
Collaborator

@basil-conto basil-conto Mar 19, 2019

Is this condition cross-platform? Or is it specific to, say, GNU Make?

Copy link
Contributor Author

@stsquad stsquad Mar 20, 2019

Good point. According to https://www.freebsd.org/cgi/man.cgi?make(1) the 0/1 distinction is the same but I couldn't see what an outright error would report. I'll see if I can find a BSD system to play with...

@stsquad stsquad force-pushed the counsel-compile-in-emacs-target-probe branch from 3a078be to 346a72e Mar 20, 2019
@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Mar 20, 2019

You seem to have included my commit from #1968, was that intended?

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 20, 2019

You seem to have included my commit from #1968, was that intended?

Oops, no. I'd just applied yours to my tree while I was reviewing. Will re-base and fix.

@stsquad stsquad closed this Mar 20, 2019
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 20, 2019

Oops, didn't mean to close the whole PR!

@stsquad stsquad reopened this Mar 20, 2019
The original code called a pipeline of shell functions to get the list
of PHONY targets. One of the consequence of this is we picked up a
bunch of noise if the make crapped out with an error. To avoid this we
bring all the text extraction into emacs with a temp buffer and only
process it if make is happy.

This is also a first step towards what we would need if we wanted to
use counsel--async-command to gather stuff and update the targets
dynamically.

---
v2
  - use bare form of (rx) for phony macro
  - tweak wording in comments
  - push/nconc/sort the results
  - revert de-hyphenating
  - let*->let
@stsquad stsquad force-pushed the counsel-compile-in-emacs-target-probe branch from 346a72e to 8426729 Mar 20, 2019
@abo-abo abo-abo closed this in b180abf Mar 21, 2019
abo-abo added a commit that referenced this issue Mar 21, 2019
* counsel.el (counsel-compile--probe-make-targets): Simplify a little.

Re #1972
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 21, 2019

Thanks, please review the follow up commit.

I'd like to avoid rx. Using a non-UNIX regex language can raise the difficulty of contributions.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Mar 21, 2019

I'd like to avoid rx. Using a non-UNIX regex language can raise the difficulty of contributions.

It's the same language, different syntax. rx makes it harder to write bad Elisp regexps, and is more readable. So if it makes any difference to the difficulty of contribution, I believe it's lowering the difficulty, rather than raising it.

@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 25, 2019

It's the same language, different syntax

By the same reasoning, all programming languages that compile to machine code are the same.
I get that the output of rx is a readable regex string. But if you wish to modify it, you either have to learn a second regex language, or replace to string and modify that.

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Mar 25, 2019

It's the same language, different syntax

By the same reasoning, all programming languages that compile to machine code are the same.

That is a false argument, because they vary greatly in their semantics, morphology, alphabets, and features.

I get that the output of rx is a readable regex string. But if you wish to modify it, you either have to learn a second regex language, or replace to string and modify that.

That's inaccurate and an exaggeration. You need only read the docstring of rx to write an equivalent, if not better-written regexp. It is far harder to understand and modify a complex regexp, regardless whether it's written as a string or rx sexp, than it is to understand how to use rx.

You personally prefer to avoid using rx, and that's perfectly fine; there are many Elispers who share the same preference. But there's no need to slander rx and blow its complexity and differences out of proportion.

astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
The original code called a pipeline of shell functions to get the list
of PHONY targets. One of the consequence of this is we picked up a
bunch of noise if the make crapped out with an error. To avoid this we
bring all the text extraction into emacs with a temp buffer and only
process it if make is happy.

This is also a first step towards what we would need if we wanted to
use counsel--async-command to gather stuff and update the targets
dynamically.

---
v2
  - use bare form of (rx) for phony macro
  - tweak wording in comments
  - push/nconc/sort the results
  - revert de-hyphenating
  - let*->let

Fixes abo-abo#1972
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
* counsel.el (counsel-compile--probe-make-targets): Simplify a little.

Re abo-abo#1972
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants