Hi Nicholas,
Le dimanche 22 janvier 2023 à 17:31, Nicholas D Steeves
<[email protected]> a écrit :
It would be nice to see some of the above text in the patch
header
and/or Debian changelog (it's not relevant for the upstream PR
as far as
I can tell)
I don't think it is relevant to this specific patch either, and it
could change in the future. I think this a generic piece of
knowledge that we have to keep in mind for all our packages. I
remember enquiring about it and later explaining it a couple of
times on #debian-emacsen, for instance. Maybe the wiki would be
the good place to store that kind of information ?
Cool approach vs skipping the tests. It would be nice to see a
couple
of words that show you've thought about why/how this approach
doesn't
hide potential bugs in real-world yasnippet usage. I hope you
believe
it's more likely that there's something weird with these three
tests
than that the tests are triggering a difficult to reproduce bug
in Emacs
;)
The bug lies most definitely in the interaction of the native
compiler and the override of the primitive `buffer-list'
implemented by the macro `yas-with-overriden-buffer-list' used by
the three failing tests. This macro is used only for testing, and
is not shipped to our users.
I sincerely hope that in a real-world yasnippet usage, our users
do not override a primitive as important as `buffer-list' so
recklessly.
Advising primitives was frowned upon even before native
compilation, but now it should be avoided at all costs. As far as
I know, the main valid use case for doing it anyway are tests that
aim to give themselves a reproducible environment, so mostly
package maintainers are concerned by this.
I do not think there is anything weird with those three tests. As
far as I can tell, `yas-with-overriden-buffer-list' implements the
override of `buffer-list' correctly, is careful enough to avoid
infinite recursion, etc... According to me, these tests do in fact
trigger a difficult to reproduce bug in emacs related to native
compilation and primitive redefinition. Our users should be fine
as long as they do not override this primitive, or if they do,
they also include it to `native-comp-never-optimize-functions'.
Go ahead and finalise the changelog
with a commit message that mentions the version
(0.14.0+git20200603.5cbdbf0d-2) as well as the
distribution/suite
(unstable)--in other words, it's just about ready to sponsor.
If you still think this fix is valid, I can go ahead. However, I
will not implement this fix with a patch, I'll use either d/rules
(like I did with projectile) or more probably d/elpa-test (that
allows arbitrary elisp evaluation before the running of the test).
Also, please forward that patch.
I'll open a PR and link the various issues and PRs you and David
contributed to.
As an aside, I seem to remember that
native-comp-never-optimize-functions
is supposed to be renamed sooner or later
Do you remember where you read that ?
I'll let you tell me whether you still think this fix is a good
idea, and I'll act accordingly.
Best,
Aymeric
