ca55bc8: Nice find, and fix, thank you! (I haven't tested it
yet, but
assume you have, that it's good, and that it will pass when I
test it)
2ff39eb: Nitpick: If you patch fixes the incompatibility, please
say so
:) Also, it may be nice to say that it's related to #1020143 if
that's
the case.
8aa6556: Thank you for noting "no changes required" because this
item
requires a human to check for compliance :)
7c4ae4a: Oh wow, this looks like something that may may have
been out of
date for years. Thanks!
b9ccd2b: What's the purpose of this digression from the way
dh_elpa does
things? It looks like it introduces potential indeterminacy.
At a
minimum, an explanation needs to be provided.
0a76135: I will not sponsor due to this action, because it's not
ok
to disable all 91 tests, when only 7 are failing
https://ci.debian.net/data/autopkgtest/unstable/amd64/y/yasnippet/28997908/log.gz
Feel skip these seven tests using another method, but please say
why
this is the correct action.
Le mercredi 4 janvier 2023 Ã 13:13, Nicholas D Steeves
<sten@debian.org> a écrit :
ca55bc8: Nice find, and fix, thank you! (I haven't tested it
yet, but
assume you have, that it's good, and that it will pass when I
test it)
I have, and I have checked that the generated HTML documentation
is the same as in currently installed versions of yasnippet (from
what I can tell, only the order of the documented symbols in the documentation changes, which could be related to changes in the
way org export works). If you think the patch is good, I'll
forward it upstream and explain my thinking.
b9ccd2b: What's the purpose of this digression from the way
dh_elpa does
things? It looks like it introduces potential indeterminacy.
At a
minimum, an explanation needs to be provided.
I just modeled the override according to what I did in my other
packages (see vertico or consult, for instance), for which I had
no objection. But it turns out that dh_elpa does "emacs -Q -L /path/to/needed/elpa/src/" rather than "emacs -q". The second one
seemed simpler, but they are not entirely equivalent, the debian
site-files do a little bit more than just adding directories to
the load-path. I will remove this commit.
0a76135: I will not sponsor due to this action, because it's not
ok
to disable all 91 tests, when only 7 are failing
https://ci.debian.net/data/autopkgtest/unstable/amd64/y/yasnippet/28997908/log.gz
Feel skip these seven tests using another method, but please say
why
this is the correct action.
The thinking was the following :
The tests' failing has nothing to do with us, a simple "rake
tests" on the upstream repo fails the same, and upstream seems
unconcerned about this. In fact, there is a PR upstream (#1125)
that solves some of the failing tests that hasn't been merged in
more than a year.
I would have liked the failing tests not to prevent the build from succeeding, but still be able to see that there are some
failing. By disabling dh-elpa-test, we make the build succeed, but autopkgtest still runs after the build, and the failure can still
be visible on ci.debian.net (which is good). If we skip the tests,
we're making them disappear from the build and from autopkgtest.
I have neither the time nor the inclination to investigate the
failing tests.
I've created the branch "temp-agon-reviewed_by_sten" which is fast-forwardable relative from "temp". This was necessary
because the
following are not doc-related...self tests for core
functionality
shouldn't break:
3 unexpected results:
FAILED basic-jit-loading
FAILED basic-jit-loading-with-compiled-snippets
FAILED visiting-compiled-snippets
(ert-deftest basic-jit-loading ()
"Test basic loading and expansion of snippets"
…
(ert-deftest basic-jit-loading-with-compiled-snippets ()
"Test basic loading and expansion of compiled snippets"
…
(ert-deftest visiting-compiled-snippets ()
"Test snippet visiting for compiled snippets."
This patch has "Forwarded: yes" in the header, so you've already
claimed
that it's been forwarded ;) It would be nice to have the URL it
was/will
be forwarded to rather than "yes" here, so that the next person
who
works on this package can track upstream discussion and status
of your
patch. I think it's likely upstream will accept it, an in the
interim I
think other people will appreciate having it made public. Ie
makes it
easier for someone to fork and apply all the important PRs to
get
yasnippet working again.
Also if for some reason the upstream project becomes
inaccessible, it's
worthwhile to have a line or two in your patch header that
explains your
thinking (for the future maintainer of the Debian package).
Sometimes
the Debian packages ends up being the only living project, and
thus the
defacto "upstream" (hopefully only until an upstream fork is
made).
Unfortunately this Debian package doesn't have an active human
maintainer, so this becomes an "if someone happens to notice and
care
about the failure" rather than the existing early warning
system. The
hard failure alerts team members and interested parties, and
correctly
removes the package from testing.
Fair point. Thanks for mentioning upstream PR. I've imported
it.
Le dimanche 8 janvier 2023 Ã 18:12, Nicholas D Steeves
<sten@debian.org> a écrit :
I've created the branch "temp-agon-reviewed_by_sten" which is
fast-forwardable relative from "temp".
Very well, I've seen the branch. Since we can rewrite history as
long as we haven't merged into master, will you let me remove my
commits from your branch rather than simply revert them ? I can do
it whenever its convenient for you, just before you merge to
master.
This patch has "Forwarded: yes" in the header, so you've already
claimed
that it's been forwarded ;) It would be nice to have the URL it
was/will
be forwarded to rather than "yes" here, so that the next person
who
works on this package can track upstream discussion and status
of your
patch.
I haven't forwarded it yet, this is exactly what I wanted to do
after you reviewed the patch. I'll open a PR on the upstream repo,
reference the issue David opened, explain my thinking there and on
the patch header as well.
Unfortunately this Debian package doesn't have an active human
maintainer, so this becomes an "if someone happens to notice and
care
about the failure" rather than the existing early warning
system. The
hard failure alerts team members and interested parties, and
correctly
removes the package from testing.
Fair point. Thanks for mentioning upstream PR. I've imported
it.
Sorry for this. What I thought would be a (not so) simple unbreak
of the documentation build turned out to be hiding other
errors. I've seen that you and David discuss it on
#debian-emacsen, I'll try and look into the remaining tests this
weekend if I have the time.
Wonderful, it looks good to me, so I've merged your work. I'll
CC you
when I ask the upstream community about the remaining three
failures.
I have a patch for the remaining three failures. It was the
trampoline compilation of the primitive buffer-list that made the
three tests fail. This is similar to what I got in projectile. I
am not exactly sure *how* the trampoline compilation makes the
test fail, but I am sure this is it.
Apologies, I haven't had time to follow this discussion very
well, but I
should note setting EMACS_INHIBIT_AUTOMATIC_NATIVE_COMPILATION=t
in the
environment may help suppress trampoline compilation.
Hello David,
Le dimanche 15 janvier 2023 Ã 07:05, David Bremner
<david@tethera.net> a écrit :
Apologies, I haven't had time to follow this discussion very
well, but I
should note setting EMACS_INHIBIT_AUTOMATIC_NATIVE_COMPILATION=t
in the
environment may help suppress trampoline compilation.
According to the function `comp-trampoline-compile', the EMACS_INHIBIT_AUTOMATIC_NATIVE_COMPILATION environment variable
does not prevent trampoline compilation, only the saving of the
output of said trampoline compilation to the file system.
According to the function `comp-trampoline-compile', the EMACS_INHIBIT_AUTOMATIC_NATIVE_COMPILATION environment variable
does not prevent trampoline compilation, only the saving of the
output of said trampoline compilation to the file system.
I just checked, EMACS_INHIBIT_AUTOMATIC_NATIVE_COMPILATION is
already set to t anyway. Sean exported it from dh_elpa_test just
before Christmas (65a588ca).
As of now, from what I understood of what I read in the upstream
mailing list and the comp.el file, the only way to truly prevent
trampoline compilation of a primitive is to add it to the variable `native-comp-never-optimize-functions'. I think it could also be
possible to prevent the trampoline compilation of *any* primitive
by setting `comp-enable-subr-trampolines' to nil.
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)
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
;)
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.
Also, please forward that patch.
As an aside, I seem to remember that
native-comp-never-optimize-functions
is supposed to be renamed sooner or later
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 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 ?
I sincerely hope that in a real-world yasnippet usage, our users
do not override a primitive as important as `buffer-list' so
recklessly.
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.
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.
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.
I agree, the wiki is a good place to centralise information.
Linking to
that article can then be used to avoid needing to reexplaining
things;
it also prevents gating when you link to it from package that
needs a
workaround--this is especially important for a package that
needs
an active human maintainer!
Another possibility might be to eventually provide additional
documentation in dh-elpa.
Thanks! That will fulfil #1 of the above.
The "not with a patch" alternative is valid but I worry it may
be
insufficient, so I would need to see what you're proposing...and
we're
short on time...for the record, here's my criteria:
1. Don't make Debian-specific fixes (make them upstream, and
make the
same changes in the Debian package), except for
Debian-specific issues
2. Provide the URL where fix was forwarded
3. It's better if no further action needs to be taken when
rebasing
the Debian package on a future (fixed) upstream version
4. Optionally provide a link to upstream bug (in this case, it
sounds
like that would be an Emacs bug). In the absence of this, it
would be
nice to see it as a TODO item, because we're supposed to be
helping
upstream work towards a more robust future.
5. Don't normalise the bug or take on technical debt--I feel
like
a simple use of d/elpa-test would probably do this, because
this file
is usually used for permanent Debian-specific CI integration
type
problems...also, if it's truly an Emacs then I think that
dh-elpa
is the place to implement the workaround.
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.
Hi Nicholas,
Le dimanche 29 janvier 2023 Ã 13:51, Nicholas D Steeves
<sten@debian.org> a écrit :
I agree, the wiki is a good place to centralise information.
Linking to
that article can then be used to avoid needing to reexplaining
things;
it also prevents gating when you link to it from package that
needs a
workaround--this is especially important for a package that
needs
an active human maintainer!
Another possibility might be to eventually provide additional
documentation in dh-elpa.
Feel free to use any part of my previous mails verbatim if you
edit the wiki (I don't have write access). If you want me to
review it first, just tell me.
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.
Done, the package should be ready to upload.
Have you already created an account, or are you waiting for
approval?
https://wiki.debian.org/FrontPage?action=newaccount
Uploaded! Thank you for your contributions and a good
discussion :)
P.S. Are you subscribed to this list, and in the future may I
follow the
usual Debian convention of not including individuals in CC?
| Sysop: | Keyop |
|---|---|
| Location: | Huddersfield, West Yorkshire, UK |
| Users: | 300 |
| Nodes: | 16 (2 / 14) |
| Uptime: | 24:43:46 |
| Calls: | 6,707 |
| Calls today: | 1 |
| Files: | 12,239 |
| Messages: | 5,352,253 |