Hi Nicholas,Le dimanche 22 janvier 2023 à 17:31, Nicholas D Steeves <sten@debian.org> 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 asI 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-functionsis 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