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

Add counsel-compile #1941

Closed
wants to merge 8 commits into from
Closed

Conversation

@stsquad
Copy link
Contributor

@stsquad stsquad commented Feb 20, 2019

My paperwork is into the fsf for copyright contribution. As you can see I've re-worked the patch as a proper series starting with some basic machinery before adding additional helpers. I would hope we can add checks for other build systems by adding a new helper and configuring the list.

I would appreciate any comments on the code especially the later commits which try and prevent counsel locking up the system if you have a lot of out of date builds. The option of using the prefix key does beg the question of if we should use the prefix to trigger the gathering of extra build types and default just to history (which would be the common case).

Currently the CI fails for older Emacsen because of the require of cl-extra. Do I need to add it somewhere in the test harness as well?

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Feb 21, 2019

Tagging @abo-abo and @Alexander-Shukaev for comments. This is a respin of #1919

Loading

counsel.el Outdated Show resolved Hide resolved
Loading
counsel.el Outdated Show resolved Hide resolved
Loading
counsel.el Outdated Show resolved Hide resolved
Loading
counsel.el Outdated Show resolved Hide resolved
Loading
counsel.el Outdated Show resolved Hide resolved
Loading
counsel.el Outdated Show resolved Hide resolved
Loading
counsel.el Outdated Show resolved Hide resolved
Loading
counsel.el Outdated Show resolved Hide resolved
Loading
counsel.el Outdated
#'(lambda (x y)
(time-less-p (nth 6 y) (nth 6 x))))))

(defun counsel-compile-get-build-subdir-builds ()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name is misleading. It essentially does what counsel-compile-get-make-invocaton but for sub-directories.

Loading

counsel.el Outdated
@@ -5216,10 +5216,13 @@ filtered out."

;;;###autoload
(defun counsel-compile (&optional arg)
"Call `compile' completing from compile history and additional suggestions."
(interactive)
"Call `compile' completing from compile history and additional suggestions.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about two dedicated functions as well?

Loading

Copy link
Contributor Author

@stsquad stsquad Mar 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of accepting a prefix I've allowed counsel-compile to be called with an explicit dir and then recurse if we go down into a build dir (although previous history is still visible on the first invocation). I'm open to better ways to handle the recursion, I wasn't quite sure how ivy's recursive support was meant to be used.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Feb 21, 2019

I reviewed every commit individually by going from the oldest to the newest so I guess the same approach should be taken to follow my comments.

Loading

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Feb 21, 2019

@Alexander-Shukaev thanks for your review comments. I'll let you know when I've respun.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Feb 21, 2019

Considering what we have now, I had a chance to think it over and I believe it is important to discuss several points before proceeding with any further implementation just to make sure that we're on the same page with respect to requirements and expectations.

First of all, there are two types of builds possible (both of which are widely used):

  1. in-source builds, where build artifacts and byproducts are written into the source tree directly during build process;
  2. out-of-source builds, where build artifacts and byproducts are written into the (dedicated) build tree during build process, while the source tree is left intact.

I personally (and most likely the majority of other developers) prefer the second approach and any build system I work with or design myself, I strive to follow that second approach. Having said that, both approaches should be supported by counsel-compile if we want it to be successful and useful.

In the context of make there are several possibilities how these approaches are implemented. In particular, cd <source-dir> && make, usually implies in-source build (PWD and CURDIR are same) unless the underlying Makefile implements out-of-source build on its own, by writing build artifacts and byproducts to some other (dedicated) build tree. In the latter case, there is not much we'd want to do about it because the out-of-source build nature is part of the underlying build system already, while in the former case, it might be the case that by doing cd <build-dir> && make -C <source-dir>, we would be able to turn the default in-source build into an out-of-source build (PWD is <build-dir> and CURDIR is <source-dir>). Some build systems won't even work with -C properly but some will explicitly support that use case. It all depends. The bottom line is that counsel-compile should make it easy to use either of these approaches. There must be notions of both configurable source directory/tree (which is not necessarily a root of the project, more on that later) and configurable (and optional) build directory/tree. cd above is only used for illustrational purposes, while as discussed in one of the review debates, we should rather let-bind default-directory to either the configured source directory tree or (if configured) build directory tree before make invocation, where in the latter case -C should additionally be used to point it to the configured source directory tree of course.

For instance, your implementation of counsel-compile-get-build-subdir-builds reminded me very much of a use case where one uses cmake to produce out-of-source builds of different configurations, e.g. Debug and Release. For example, my cmake-based build system does that by default. It creates .build directory in the root of the project and generates the corresponding build trees either under Debug or Release subdirectories, where afterwards one could either cd .build/Debug && make or make -C .build/Debug (either way the build is already out-of-source). However, I'm a bit confused now because it seems to me that you're mixing the two approaches given that you by default propose to just make in project root, while also support counsel-compile-get-build-subdir-builds for something that looks like out-of-source builds. Am I missing anything here? Could you describe your use cases for counsel-compile-get-build-subdir-builds and where build artifacts/byproducts go in this case?

Secondly, root of the project is not necessarily a source directory tree root. I think scanning upwards for a dominating files like .git and etc. is a good idea to determine a project and that's what most packages do in this space but we actually have something better here. We already know what we are looking for and these are, for example, Makefiles. That is, for example, we could search upwards for a topmost Makefile within a directory containing .git (if such directory exists, but if it does not, while Makefile does, it should also be OK). Furthermore, .git or .hg or whatever else there is, are also not by any means an ultimate indication of a buildable project. Take for example cmake-based projects, these will typically contain CMakeLists.txt, where one first has to generate the native build system by running cmake. Take my example above, I generate it into .build plus subdirectory indication configuration and only then build it with make in one of these subdirectories. Both CMakeLists.txt and .build could also be indicators of buildable project root. At the same time, somebody could use /tmp/my-mega-project-build to build some project in /home/me/projects/my-mega-project out-of-source, e.g. again with cmake simply by running cd /tmp/my-mega-project-build && cmake /home/me/projects/my-mega-project, which will generate the native build system (e.g. Makefiles) in the /tmp/my-mega-project-build build tree essentially making this an out-of-source build, and then all what's left is to either cd /tmp/my-mega-project-build && make or make -C /tmp/my-mega-project-build to produce actual build artifacts in /tmp/my-mega-project-build. That's why tracking/configuring both build tree and source tree is important.

Loading

@basil-conto
Copy link
Collaborator

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

I haven't read the code yet, but reading through the discussion so far made me think of the project.el library that was introduced in Emacs 25. In its current state, it is like an early and less featureful but more programmable and better integrated projectile.el. In particular, its API is implemented with cl-lib generic functions so it can easily be extended with different methods for different purposes. One of the first successful users of this library is the excellent Eglot LSP client, for example.

If project discovery and manipulation is so central to the feature(s) being proposed in this PR, I would highly recommend looking into project.el and trying to reuse its API. It would obviously be better if the proposed compilation system worked on older Emacsen as well, but personally I wouldn't mind requiring Emacs 25+ or 26+ in exchange for better integration with project.el and a cleaner/simpler implementation. Kudos for optionally supporting both, of course. ;)

Loading

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Feb 22, 2019

@basil-conto certainly I started down this path because the tool I used previously (eproject) has been un-maintained for some time and re-integrating my changes into a new project framework seemed excessive given my relatively modest needs. Now that mainline Emacs has project support I can certainly look at using project.el APIs to augment the choices made. I suspect however that @abo-abo would want that to be optional so we maintain our support for 24.3 and up. Any tips on managing that gracefully would be useful.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Feb 22, 2019

I gave it some more thought and I think we should somehow outline the exact requirements with some details of what we want counsel-compile to do given all the inputs so far:

  1. This will cover the vast majority of the use cases. A user runs either counsel-compile or compile (directly) in some buffer. counsel-compile should hook into compile (e.g. advice) and record two things: the compilation command itself and the default-directory. Now that it knows the directory where the command was run, it can decide to which history to append the command. First it can search upwards from that directory to attempt these project source tree root heuristics and if successful, it should append this command into that "bucket". Otherwise, it should create a new bucket with that default-directory itself and append that command there so that any buffers in sub-directories would also see that command as part of their history regardless of the fact that it does not satisfy project source tree root heuristics. I think the alist that I've mentioned in one of the review debates above can serve as a data structure to implement this "directory-bucketed" compilation command history.

  2. This will cover out-of-source build use cases. So when user runs some build command in a source tree and we can record it, that's easy and covered by the previous item. But the user may also run a build command in another directory, which is in fact a build tree directory of some project. Ideally, we would want such commands to also be appended to the project's compilation history. Such command can be run with either:

    • M-x compile cd <build-dir> && make;
    • M-x compile make -C <build-dir>;
    • changing to some buffer with default-directory being <build-dir> and doing M-x compile make.

    In all these cases, counsel-compile can also record <build-dir> (either by parsing the command or by directly capturing default-directory). Now as long as there is a customization variable that allows one to specify a list of build tree directories per project source tree directory (the data structure very similar to how command history can be implemented), counsel-compile can check whether <build-dir> matches some project and append the command to the corresponding history bucket accordingly. If nothing matches, then fall-through to the "otherwise" case of the previous item to create a new history bucket for default-directory of the current compilation command.

  3. It is very important to always give relevant completion candidates. In the context of compilation that's definitely the MRU approach. Maybe @abo-abo or @basil-conto can comment whether MRU sorting has to be implemented by counsel-compile itself (given the complexity of the history data structure) or we could already benefit from :history somehow. That is I'm not even sure we could just plug our bucketed history data structure into :history.

  4. Additional completion candidate sources, such as parsing PHONY targets and build sub-directory heuristics should definitely be optional and not included by default as they will overwhelm the number of choices and people can get lost. Let users customize that part themselves, when they understand how counsel-compile functions. The implementations of those can still be offered of course, just not used by default. I'm still hesitant about build sub-directory heuristics as the current implementation seems to rely on too many assumptions. Furthermore, these could simply be treated as out-of-source builds again and handled as part of the second item in a "generic" way, i.e. no matter whether they are sub-directories or how much nesting they have. In any case, let's come back to this optional feature in the very end when the first three items are implemented as they seem to be the most useful.

Feel free to join the discussion, I'm looking forward to your feedback and use cases.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Feb 22, 2019

Regarding the data structure for directory-bucketed command history, there are two approaches:

  1. The one that you used: linear history as a list on top of which you keep applying the filtering algorithm. It looks like this can work well with the existing ivy framework. We only need to find out about MRU sorting. Either ivy should maintain that sorting or counsel-compile every time it is invoked. If the history variable it self is MRU sorted, then even filtering on top of it will yield the correct subset of sorted candidates.
  2. The alist-based structure I proposed but I don't know whether ivy framework would even support that via :history keyword as mentioned. Somebody needs to comment on this. The advantage is that no filtering is needed in this case or rather only a lookup, which is certainly faster than filtering the whole history all the time.

Loading

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Feb 22, 2019

@Alexander-Shukaev thanks for your thoughtful use-case analysis. A couple of questions:

For 2. would that entail us advising the normal M-x compile and filling in counsel-compile-history with the appropriate annotations? If you say navigate to a build directory outside the tree you would need to a) spot the Makefile is a link and b) follow it and see it's attached to a source project directory where we are keeping all the history. I think you mentioned before that really the Makefile is the indicator of where the source comes from.

For 4. I think I'm already coming to the view it should not be the default, maybe triggered by a C-u invocation or have a separate command. However a more intuitive user experience would be to defer the magic guessing until the user had specified which build directory they want to build in - this saves computing many subtle but different variations ahead of time. I'm not sure if I've seen such a multi-stage approach before but I guess maybe hitting tab should prompt for more completions? Are there counsel commands that do this already?

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Feb 22, 2019

@stsquad, regarding 2, I think advising compile in general is a good start of the implementation. This is the central place to record the information we need for counsel-compile. Occasionally, people would just do M-x compile in some buffer for whatever reason and I think it makes sense to record that as part of the history for counsel-compile. You'd also not have to implement that logic in counsel-compile as that already uses compile under the hood. Regarding points a) and b), I think I talked too much :) and might have confused you. Let me reiterate the logic here as it's a bit tricky to convey it clearly in a prose (apologies, English is not my native). Whenever you record a command and default-directory in compile, you first try to establish what is likely a "build" directory. That is following the three possible scenarios from point 2, you parse the command for cd and -C (caution as this might be fragile and will not work in 100% of cases, e.g. what if we have cd ${SOME_DIR} ..., this would need different treatment, but the important part now is the idea itself so let's try), if you find a match and extract a valid directory, then this is the "build" directory, if not, then fallback to treat default-directory as "build" directory. Now you go to a customizable variable which is an alist, where car is a project source tree root and cdr is a list of that project's build tree roots (that should be specified by user himself if he wants that feature), and you do a "backward" search here, i.e. you look for a car whose cdr (list) contains "build" directory (or even maybe if the current "build" directory is a sub-directory of one of the directories in that list). If you have a match, then the car gives you the project source tree directory which you now can use to record the current command to (in another history variable that we discussed already). If you don't have a match, then all you can do is, well, follow the algorithm from point 1, i.e. think of that "build" directory not as some external build tree of some project stored elsewhere but rather as a potential project source tree itself, and so you do all that business by upward searching for dominating files/directories to modify the history of that or if not found, create a brand new history of the "build" directory itself (that "Otherwise" part of the point 1) as the last reasonable fallback. Keep in mind that while the part of considering default-directory is general, the part of parsing the command for cd and -C is build-system-specific and hence needs to be hookable. That is we should definitely provide the default implementation for make as both reference and convenience, but it should also be easy for users to do the same for any build system and plug it as a part of counsel-compile pipeline.

Regarding 4, that was exactly my point. Let users decide which candidates they want and make it completely configurable per project. I do believe that point 2 actually already solves that problem in a general way. That is as long as users list those sub-directories as potential build directories and once run a command (whatever it is) which refer to one of these build directories according to our algorithm from point 2, then those will be added to the history of this project and that's all we want.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Feb 22, 2019

Regarding data structure of history, I thought of one more advantage of the approach you took (linear list plus filtering), where I think counsel-compile should provide two functions:

  1. the most frequently used one, which returns filtered history per project;
  2. less frequently used one, which returns non-filtered "global" history for all projects.

Loading

@basil-conto
Copy link
Collaborator

@basil-conto basil-conto commented Feb 24, 2019

@Alexander-Shukaev

counsel-compile should hook into compile (e.g. advice) and record two things: the compilation command itself and the default-directory.

It should hopefully be possible to record/obtain this information without advice. Advice should only be used by libraries when there is no alternative, or when an API has been specifically designed to work with advice, e.g. whitespace-enable-predicate.

It is very important to always give relevant completion candidates. In the context of compilation that's definitely the MRU approach. Maybe @abo-abo or @basil-conto can comment whether MRU sorting has to be implemented by counsel-compile itself (given the complexity of the history data structure) or we could already benefit from :history somehow. That is I'm not even sure we could just plug our bucketed history data structure into :history.

I don't see why we can't use a standard minibuffer history list, at least to begin with. It can always be made more sophisticated later on if usage shows a need for that. But I haven't been following this discussion, so apologies if my comment is useless.

Loading

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Feb 24, 2019

Certainly M-x compile has it's own compile-history variable which I can certainly add a helper for (possibly utilising the heuristics we are going to drop for counsel-compile-history due to better meta data).

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Feb 25, 2019

@basil-conto, I'm not sure how would you record default-directory unless there are hooks executed during compile, which there should be theoretically.

@stsquad, compile-history is a good candidate for your initial linear history implementation. However, to be consistent with the rest of the counsel, counsel-compile has to maintain its own history variable. Plus, as we discussed earlier, apart from just a compilation command, you need to record default-directory, preferably by propertizing the command string itself. Although this can certainly work with compile-history directly, this could still look surprising to users viewing the contents of this variable.

Loading

@basil-conto
Copy link
Collaborator

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

@Alexander-Shukaev

I'm not sure how would you record default-directory unless there are hooks executed during compile, which there should be theoretically.

I'm not sure what you're trying to do, but *compilation* buffers have a buffer-local value of default-directory, and compile sets compilation-directory globally. And there are hooks, of course.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Feb 25, 2019

I checked the code and I think compilation-start-hook sounds like a good place to record that information. See compilation-start, it has the following before running this hook:

    (set (make-local-variable 'compilation-directory) thisdir)
	(set (make-local-variable 'compilation-environment) thisenv)

Haven't thought of it, but it could also be important to record compilation-environment as well since that's what compile is remembering itself. To summarize, at compilation-start-hook, you could write counsel-compile-history with command and propertize it with compilation-directory and compilation-environment.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Feb 25, 2019

Keep in mind the following interesting documentation remark:

(defun compilation-start (command &optional mode name-function highlight-regexp)
  "Run compilation command COMMAND (low level interface).
If COMMAND starts with a cd command, that becomes the `default-directory'.
The rest of the arguments are optional; for them, nil means use the default.

It looks like compilation-start is already doing part of the heuristics that we discussed earlier.

Loading

stsquad added 2 commits Mar 10, 2019
This provides the initial framework for counsel-compile. The building
of the various compile options is driven by a variable
counsel-compile-local-builds which provides a list of things that can
be used to build up the available compile commands. The list can
contain functions, strings or even just lists of options.

Currently just set to the default "make -k"

---
v2
  - fix setup to set up
  - accept atoms from funcall and listify
  - nconc the list
Introduce a "private" helper which can be used for general project
root finding and then use it for counsel-locate-git-root.
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 11, 2019

If project discovery and manipulation is so central to the feature(s) being proposed in this PR, I would highly recommend looking into project.el and trying to reuse its API. It would obviously be better if the proposed compilation system worked on older Emacsen as well, but personally I wouldn't mind requiring Emacs 25+ or 26+ in exchange for better integration with project.el and a cleaner/simpler implementation. Kudos for optionally supporting both, of course. ;)

There doesn't seem to be a public API to find a project root, although you could certainly plug in project-current to the new defcustom counsel-compile-root-function

Loading

@basil-conto
Copy link
Collaborator

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

There doesn't seem to be a public API to find a project root, although you could certainly plug in project-current to the new defcustom counsel-compile-root-function

WDYM there's no public API? What about project-current, project-find-functions, project-roots, and project-external-roots?

Loading

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 11, 2019

@basil-conto I mean project-current is in interactive function so probably not suitable for plugging into the ivy setup - it calls project--find-in-directory which is much like the counsel equivalent but "private" to project.el. Either way project-current doesn't seem to return a guess for the project root unless you have declared it as such so we can't default to using it. However you can certainly plug in your own helper into the defcusom.

Loading

@basil-conto
Copy link
Collaborator

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

I mean project-current is in interactive function so probably not suitable for plugging into the ivy setup

That it's interactive says nothing about its place in the API. project-current is actually the foundation of the whole API, as called out in the library's commentary: http://git.savannah.gnu.org/cgit/emacs.git/tree/lisp/progmodes/project.el#n37

;; Infrastructure:
;;
;; Function `project-current', to determine the current project
;; instance, and 5 (at the moment) generic functions that act on it.
;; This list is to be extended in future versions.

Either way project-current doesn't seem to return a guess for the project root unless you have declared it as such so we can't default to using it.

Sorry, I don't understand what you mean. Declared what as what? Why can't we configure project-find-functions for our purposes and/or use transient project instances?

However you can certainly plug in your own helper into the defcusom.

Again, not sure what you mean. Which defcustom are you referring to? What would this helper achieve?

Loading

stsquad added 5 commits Mar 12, 2019
To avoid the history polluting the current project we provide a
filtered history that weeds out any paths outside of the current
project directory. To do this we use metadata embedded in the
counsel-compile-history which should embedded the source and build
directories.
This adds a helper which can provide all the potential make
invocations for a given makefile. It differs slightly from the example
in helm-make in that it only considers PHONY targets which are
generally the top level "meta" targets of a makefile. This is still
quite a big list on some projects. For example QEMU provides 394 such
targets.

We could probably shorten the list somewhat by only considering PHONY
targets which we not in themselves prerequisites of other PHONY
targets but so far I've not implemented that as the additional
processing may be a performance issue.

The other differences from helm-make's approach is we do the
extract with plain shell pipes and sort the final result.

---
v2
  - defcustom for counsel-compile-make-args
  - defcustom for make pattern
  - use faces to visually separate blddir
This helper supports the common practice of using a build directory to
support multiple configurations of the build. This is often done to
avoid recompiling all build variants of a project when doing
development work.

---
v2
  - use recursive and cmd properties
We can't use ivy-read's history directly as we loose our propertized
strings. We also want to get useful information from M-x compile which
will do things like parsing for cd and other heuristics we'd rather
not complicate our lives with.

Repeatedly calling add-hook on each invocation is a little inelegant
but it's hardly going to show in the numbers.
@stsquad stsquad force-pushed the add-counsel-compile-rfc branch from 68cb253 to df6f188 Mar 12, 2019
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 12, 2019

@basil-conto see https://github.com/stsquad/swiper/blob/add-counsel-compile-rfc/counsel.el#L5125 which is used to find the project root. You could plug in project.el there but you seem to be arguing for using project.el APIs directly within counsel which might make sense in a few years once everyone has project.el support, otherwise you'd need a bunch of detection code and fall-back.

Loading

@stsquad stsquad marked this pull request as ready for review Mar 12, 2019
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 12, 2019

@abo-abo and @Alexander-Shukaev ok I've finished the re-base and clean-up. Big pictures changes:

  • hooks into compilation-start-hook for history
  • a bunch of stuff is now defcustom for easier tweaking
  • subdir builds are handled as a multi-stage M-x counsel-compile
  • heavy use of text properties for handling meta-data (no more parsing of cd)

Loading

@basil-conto
Copy link
Collaborator

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

@basil-conto see https://github.com/stsquad/swiper/blob/add-counsel-compile-rfc/counsel.el#L5125 which is used to find the project root. You could plug in project.el there but you seem to be arguing for using project.el APIs directly within counsel which might make sense in a few years once everyone has project.el support, otherwise you'd need a bunch of detection code and fall-back.

Perhaps I was not clear, but my last few messages have been in response to your claim that project.el lacks an API for finding the root of a project. My argument is that this is clearly not true, given the functions and user option I listed previously.

The variable counsel-compile-root-function can default to a project.el implementation when the library is present in newer Emacsen, and can otherwise fall back on counsel-locate-git-root or whatever else we have, no? I haven't given this much thought, but off the top of my head I don't foresee the kind of complexity you seem to be worried about.

Loading

@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 12, 2019

If I:

(setq counsel-compile-root-function 'project-current)

And then invoke M-x counsel-compile it fails as it returns 'nil so it needs further setting up whereas counsel-locate-git-root works in the common case, assuming of course the user is using git.

Loading

Add a smarter project root function that tries a series of steps from
most emacsey to least emacsey.
@stsquad
Copy link
Contributor Author

@stsquad stsquad commented Mar 12, 2019

@basil-conto ok how about e7875ac?

Loading

@stsquad stsquad changed the title Add counsel compile [RFC] Add counsel-compile Mar 12, 2019
@abo-abo abo-abo closed this in 5f5a263 Mar 12, 2019
abo-abo added a commit that referenced this issue Mar 12, 2019
@abo-abo
Copy link
Owner

@abo-abo abo-abo commented Mar 12, 2019

Merged, thanks. I squashed your commits into one and added a clean up commit on top, please review it.

Note this change, otherwise the code was throwing an error for me:

-(project-current)
+(cdr (project-current))

Is project-current different on your Emacs version?

(add-hook 'compilation-start-hook 'counsel-compile--update-history)

I would prefer to avoid this code. Can counsel--get-compile-candidates call counsel-compile--update-history instead?

Loading

abo-abo added a commit that referenced this issue Mar 12, 2019
(let ((srcdir (get-text-property 0 'srcdir hist))
(blddir (get-text-property 0 'blddir hist)))
(when (or (and srcdir (string-match srcdir root))
(and blddir (string-match blddir root)))
Copy link
Collaborator

@basil-conto basil-conto Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are these string-match calls supposed to do? How can you safely interpret file names as regexps? Perhaps you meant to use file-equal-p or file-in-directory-p instead?

Loading

Copy link
Contributor Author

@stsquad stsquad Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@basil-conto ahh I failed to find those - file-in-directory-p sounds like a better bet.

Loading

`counsel-compile-local-builds'."
(let ((cands))
(if (stringp counsel-compile-local-builds)
(setq cands (list counsel-compile-local-builds))
Copy link
Collaborator

@basil-conto basil-conto Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this stringp check necessary? The docstring of counsel-compile-local-builds does not mention the possibility for this variable to take on a string value.

Loading

Copy link
Contributor Author

@stsquad stsquad Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should enforce it is always a list in the defcustom?

Loading

Copy link
Collaborator

@basil-conto basil-conto Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we should enforce it is always a list in the defcustom?

I think that would be overkill. If users don't follow instructions, it's their own fault. Besides, it's a defvar, not (yet) a defcustom. I just wanted to check with you that counsel-compile-local-builds was indeed intended as a list, rather than a string or list.

Loading

(directory-files-and-attributes blddir
t (rx (not (in "." ".."))))
(lambda (x y)
(time-less-p (nth 6 y) (nth 6 x))))))
Copy link
Collaborator

@basil-conto basil-conto Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docstring says access time (which is the 5th file attribute), yet this sorts by status modification time (which is the 7th file attribute). Which one is correct?

Loading

Copy link
Contributor Author

@stsquad stsquad Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we want modification time (as in the last time a build was run) so the fix is to the docstring.

Loading

Copy link
Collaborator

@basil-conto basil-conto Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The last time a file's contents were changed is stored in file-attribute-modification-time, AKA (nth 5 ATTRS). The last time either the contents, access mode bits, owner, group, etc. were changed is stored in file-attribute-status-change-time, AKA (nth 6 ATTRS). Which of the two is important here?

Loading

Copy link
Collaborator

@basil-conto basil-conto Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which of the two is important here?

On further thought, I think file-attribute-modification-time.

Loading

@Alexander-Shukaev
Copy link

@Alexander-Shukaev Alexander-Shukaev commented Mar 13, 2019

I briefly looked at it and I have some comments. Will find some time tomorrow to enter the discussion.

Loading

@stsquad stsquad deleted the add-counsel-compile-rfc branch May 1, 2019
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
This provides the initial framework for counsel-compile. The building
of the various compile options is driven by a variable
counsel-compile-local-builds which provides a list of things that can
be used to build up the available compile commands. The list can
contain functions, strings or even just lists of options.

Currently just set to the default "make -k"

---
v2
  - fix setup to set up
  - accept atoms from funcall and listify
  - nconc the list

counsel.el (counsel-locate-git-root) more common functions

Introduce a "private" helper which can be used for general project
root finding and then use it for counsel-locate-git-root.

counsel.el (counsel-compile): add filtered compile history

To avoid the history polluting the current project we provide a
filtered history that weeds out any paths outside of the current
project directory. To do this we use metadata embedded in the
counsel-compile-history which should embedded the source and build
directories.

counsel.el (counsel-compile): add make completion

This adds a helper which can provide all the potential make
invocations for a given makefile. It differs slightly from the example
in helm-make in that it only considers PHONY targets which are
generally the top level "meta" targets of a makefile. This is still
quite a big list on some projects. For example QEMU provides 394 such
targets.

We could probably shorten the list somewhat by only considering PHONY
targets which we not in themselves prerequisites of other PHONY
targets but so far I've not implemented that as the additional
processing may be a performance issue.

The other differences from helm-make's approach is we do the
extract with plain shell pipes and sort the final result.

---
v2
  - defcustom for counsel-compile-make-args
  - defcustom for make pattern
  - use faces to visually separate blddir

counsel.el (counsel-compile): add build dir helper

This helper supports the common practice of using a build directory to
support multiple configurations of the build. This is often done to
avoid recompiling all build variants of a project when doing
development work.

---
v2
  - use recursive and cmd properties

counsel.el (counsel-compile--update-history): add compilation hook

We can't use ivy-read's history directly as we loose our propertized
strings. We also want to get useful information from M-x compile which
will do things like parsing for cd and other heuristics we'd rather
not complicate our lives with.

Repeatedly calling add-hook on each invocation is a little inelegant
but it's hardly going to show in the numbers.

doc/ivy.org: add counsel-compile to sample bindings

counsel.el (counsel-compile): add counsel-locate-project-dwim

Add a smarter project root function that tries a series of steps from
most emacsey to least emacsey.

Fixes abo-abo#1941
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
astoff added a commit to astoff/swiper that referenced this issue Jan 1, 2021
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

4 participants