I am attaching a slightly tweaked version of the patch that will make
less use of internet connection (use only when really, really
necessary), fixes variable expansion to be done once instead of in every
make sub-process, and adds some extra info messages.

Max Nikulin <manikulin@gmail.com> writes:

> Ihor, do added makefile rules follow best practices used by other Emacs 
> packages in respect to dependencies?

I know no other Emacs packages that manage dependencies using make.

> I do not like the idea of network queries on every make.

Any better suggestions?

> In some cases I would prefer to specify a directory where compat.el is
> installed, so Makefile should not try to manage this directory.

Sure. See EFLAGS variable. Makefile will not try to manage
-L /path/to/compat directory.

> Originally I expected that either compat.el would be included into Org 
> repository either as a copy of the file or as git submodule.

That will create maintenance nightmare.

> In addition I am afraid of recursive removal of directories. It is too 
> easy to remove too much.
>
>> +pkgdir = $(shell pwd)/pkg-deps
>
> Make has CURDIR variable, but I am unsure if it is safe to use it in 
> this context.

Actually, we need pkgdir := $(shell pwd)/pkg-deps.
CURDIR is wrong because default.mk will trigger evaluation in every make
sub-process as well.

>> +	-$(FIND) $(pkgdir) \(  -name \*.elc \) -exec $(RM) {} +
>
> find has -delete action. I see that "-exec $(RM)" is already used.

I just copied the existing code.
I guess we can use -delete as well. I do not have a lot of experience
with this usage of find. May you share the equivalent find call with
-delete that works with this patch?

>> +cleanpkg:
>> +	-$(RMR) $(pkgdir)
>
> Perhaps it is impossible to completely avoid recursive deleting of 
> directories, but I still afraid of cases like
> https://github.com/MrMEEE/bumblebee-Old-and-abbandoned/issues/123
> https://news.ycombinator.com/item?id=9254876

I do not think that we have any significant risk here.
I would not mind alternative way to implement clean target though. If
you know one.

>> Subject: [PATCH 2/3] Use compat.el library instead of ad-hoc compatibility
>>  function set
>
> It would be easier to review if this patch was split into 2 parts:
> - add compat.el dependency (unused)
> - replace functions to ones from compat.el

Sure, but after spending half an hour trying to decouple this part, I
gave up and decided to leave it as is. And there is nothing fancy in
adding compat dependency in 2/3 patch anyway. Just one (require 'compat
nil t), two macro definitions adviced by compat manual (copy-paste from
compat code), and adding compat to EPACKAGES in makefiles.