Re: Finalizing 'inhibit-automatic-native-compilation'

[Aymeric Agon-Rambosson <aymeric.agon@yandex.com>]

Hello everyone, and sorry for my being late.

Le jeudi 2 février 2023 à 09:17, Sean Whitton 
<spwhitton@spwhitton.name> a écrit :

> Aymeric, you investigated one of these cases in details.  Would 
> you be
> able to summarise the situation we had in Debian for Eli, 
> please?

The case Sean refers to is probably the packaging of projectile 
(https://github.com/bbatsov/projectile).

In the testbed of this package, the buttercup library 
(https://github.com/jorgenschaefer/emacs-buttercup) is used to 
advise some primitives, like `file-exists-p', 
`insert-file-contents', `file-directory-p' and probably 
others. These primitives can be advised so as to be replaced by a 
function that does nothing, or that always returns a specific 
value, etc...

The following test was interesting :

(describe "projectile-parse-dirconfig-file"
  (it "parses dirconfig and returns directories to ignore and 
  keep"
    (spy-on 'file-exists-p :and-return-value t)
    (spy-on 'file-truename :and-call-fake (lambda (filename) 
    filename))
    (spy-on 'insert-file-contents :and-call-fake
            (lambda (filename)
              (save-excursion (insert 
              "\n-exclude\n+include\n#may-be-a-comment\nno-prefix\n 
              left-wspace\nright-wspace\t\n"))))
    (expect (projectile-parse-dirconfig-file) :to-equal 
    '(("include/")
                                                          ("exclude"
							   "#may-be-a-comment"
							   "no-prefix"
							   "left-wspace"
							   "right-wspace")
                                                          nil))
    ;; same test - but with comment lines enabled using prefix '#'
    (let ((projectile-dirconfig-comment-prefix ?#))
      (expect (projectile-parse-dirconfig-file) :to-equal 
      '(("include/")
							    ("exclude"
							     "no-prefix"
							     "left-wspace"
							     "right-wspace")
							    nil)))
    ))

The primitive `file-exists-p' is advised as to always return t, 
whether the argument corresponds to an existing file or not. If we 
assume the trampoline is not already there (and that is the case 
in our build environment), a trampoline compilation is triggered, 
and exits without error. The advice is effective only after that.

Then `file-truename' is advised, but it is not a primitive.

Then `insert-file-contents' is advised as well, it is a 
primitive. We enter the function `comp-subr-trampoline-install', 
and we begin by checking whether the corresponding trampoline 
already exists by entering the function 
`comp-trampoline-search'. This function relies on `file-exists-p', 
which answers that the file corresponding to the compiled 
trampoline exists, even if it doesn't. Hence we enter 
`native-elisp-load' with a filename that doesn't exist as 
argument, and we error out :

comp-subr-trampoline-install(insert-file-contents)
comp-trampoline-search(insert-file-contents)
native-elisp-load("/tmp/buttercupKuLmvD/28.2-4001e2a9/subr--trampoline-696...
error: (native-lisp-load-failed "file does not exists" 
"/tmp/buttercupKuLmvD/28.2-4001e2a9/subr--trampoline-696e736572742d66696c652d636f6e74656e7473_insert_file_contents_0.eln")

We always get the error because the trampoline is never there 
beforehand, and upstream did not get it precisely because it was 
lucky to always have it before running the test. If we have to 
load *any* trampoline after the advice of `file-exists-p' is 
effective, and we're not lucky enough to already have the 
trampoline, then we reach the error.

In that case, we worked around the problem by adding the primitive 
`insert-file-contents' to the variable 
`native-comp-never-optimize-functions'.

>>   . where and under what circumstances will those advised 
>>   functions be
>>     called, as part of your preparation of the packages?  (if 
>>     the
>>     advised functions are only called when the end-user uses 
>>     the
>>     package, that is not relevant to the present discussion)

As far as we can tell, this advising of primitives happens mostly 
in tests, that is in our build environment on our build machines.

>>   . if we provide a way to specify, via 
>>   comp-enable-subr-trampolines,
>>     the directory to which the trampolines will be written, 
>>     will that
>>     satisfy your uses? if not, why not (details, please)?
>>   . why cannot you use native-comp-eln-load-path to force the
>>     trampolines go to a directory of your choice?

In this case, this is not pertinent. We need to :
- either not to have to install a trampoline
- or, if we have to, be certain to find it in the *first* 
  directory returned by `comp-eln-load-path-eff'.

If neither condition are met, we reach the error. The only 
variables that are of help here are 
`native-comp-never-optimize-functions' or 
`comp-enable-subr-trampolines'.

Since EMACS_INHIBIT_AUTOMATIC_NATIVE_COMPILATION acts on neither 
of these, we have to manually add

(with-eval-after-load 'comp (push 'insert-file-contents 
native-comp-never-optimize-functions))

At the beginning of the test file.

For this reason, it would be nice, if possible, that 
EMACS_INHIBIT_AUTOMATIC_NATIVE_COMPILATION also disables 
trampoline compilation, for instance by setting 
`comp-enable-subr-trampolines'. The name of the environment 
variable would be changed accordingly, of course.

The performance penalty wouldn't be a problem, since this would 
only be for the duration of the test. If Andrea prefers, we could 
also have two different environment variables, one for trampoline 
and the other for deferred async compilation. But I don't see why 
only one knob doesn't work : if we had an environment variable 
that acted on both `inhibit-automatic-native-compilation' and 
`comp-enable-subr-trampolines', like what happens when 
`native-comp-available-p' evaluates to nil, wouldn't that exactly 
"really disable native compilation", or am I missing something ?

>> I dislike having environment variables that alter Emacs 
>> behavior,
>> because environment variables are inherited by sub-processes.

This is precisely why I like it. Our build mechanism can have 
emacs instances nested deep in layers of wrapper scripts. Because 
of this, we can simply export the variable in the environment of 
the ancestor process, and not worry about adding --eval arguments 
in various places in the middle of our scripts. But only the 
descendants of this ancestor process are concerned by this 
environment variable, and they live only as long as the building 
mechanism runs, so I don't see the reason for worry. Then, if 
users want to use this variable in normal use, and they decide on 
top of that to have multiple nested emacsen, then I agree that 
they have to be careful, but that situation seems a bit 
far-fetched.

I hope everything was clear.

Best,

Aymeric Agon-Rambosson

P.S. We have a couple of packages for which we have deactivated 
some trampoline compilation in order to pass tests or avoid an 
error. I haven't been able to find out why like I did with 
projectile, but I guess these cases can be of interest to you :

https://github.com/joaotavora/yasnippet/pull/1158

https://github.com/DamienCassou/beginend/pull/75

P.P.S. The new mechanism consisting in having all the trampolines 
compiled as part of the building process would probably solve 
these two problems, but not the one from projectile. I assume 
these trampolines would be put in the system eln cache, which is 
not returned as the first directory by 
`comp-eln-load-path-eff'. We would therefore reach the error 
anyway. But this is very good in my opinion, as are all efforts to 
increase the amount of stuff native compiled eagerly (like 
NATIVE_FULL_AOT).