Hello team,At Sean's request, I reproduce here a modified version of my mail to him regarding the packaging of the latest emacs release, and more specifically about patches 12 and 13.
The discussion with upstream we've had in February regarding inhibit-automatic-native-compilation, and the environment variable EMACS_INHIBIT_NATIVE_COMPILATION, should provide useful context.
tl;dr : The changes introduced by Andrea Corallo following said discussion have made patches 12 and 13 (the rebased version of Lars Ingebrigsten commits) useless, so we should drop them.
In fact, I thought that this was the unanimous conclusion of the discussion, hence my mail to Sean from yesterday where I enquired as to why patches 12 and 13 were still present.
For context, the earlier version of patches 12 and 13 were introduced to solve a series of FTBFS bugs we had with various elpa-* packages (see #1021842, for instance). Those bugs happened roughly like so :
1. Whenever a primitive (a function available from elisp, but implemented in C) is advised, it is necessary to compile a trampoline to allow to jump dynamically to the native-compiled definition of the advice, which could be anywhere on the user's system.
2. Said trampoline needs to be output to a file.3. The job of function comp--trampoline-abs-filename is to find a directory in which to store this file, and return it. 4. This search was made from a list of candidates (think /usr/lib/emacs/29.1/native-lisp or ~/.emacs.d/eln-cache). 5. If no such candidate was available, either because they were unwritable (the former) or because they did not exist (the latter, in our build environments), the function errored out, which caused our bug.
The commits by Lars (5fec9182dbe and f97993ee667f), that Sean backported as patches 12 and 13, solved the problem for us like this : 1. When we enter the function comp--trampoline-abs-filename, we leave it immediately if inhibit-automatic-native-compilation is true.
2. Hence we never reach the loop, so we never reach the error.3. Because the function comp--trampoline-abs-filename returns nil, the trampoline is output to a temporary file.
The commit by Andrea (ce4a066ed), that is present in 29.1, solves the problem for us like this :
1. We reach the loop normally.2. If at the end of the loop, we haven't found any suitable directory candidate for trampoline output, rather than erroring out, a temporary directory is chosen. 2bis. This temporary directory is guaranteed to always exist, even in our sbuild environments (basically a directory in /tmp, and /tmp *has* to exist on linux systems). 3. comp--trampoline-abs-filename returns with this directory, the trampoline is therefore output to a file in this temporary directory.
In conclusion, both patches have exactly the same effect at the end of the day, namely outputting the trampoline to a temporary file :
- Lars, if we ask for it via inhibit-automatic-native-compilation. - Andrea, automatically as a last resort.The rebased version of the patches 12 and 13 combines both solutions in the same function, despite the fact that they have the same effect, and are exclusive of each other (there is no path inside the function that allows to reach both of them).
I think that we should drop Lars' patches, and rely on Andrea's solution. Less patches for us to maintain, for an otherwise equivalent behaviour.
I wanted to make absolutely sure that dropping Lars' patches did not cause the FTBFS bugs to come back.
For this reason, I have in my fork (https://salsa.debian.org/ricorambo/deb-emacs) a branch (rico-without-lars-patch) that simply drops patches 12 and 13.
The experiment is the following : for every package maintained by the debian-emacsen team, a build is attempted with the version of emacs in current sid, and another is attempted with the version of emacs from my fork.
If the patches were really useful, there would be at least some elpa-* package that would build correctly on the first attempt, and FTBFS on the second.
There is none, in any of the 340-something packages we maintain. The patches make no difference for every single one of our team-maintained patches.
I include the script that will allow you to repeat the experiment. If you run it sequentially, it will take quite some time. Try to run it in parallel like so :
cat paquets_debian_emacsen | parallel test_emacs.sh > results
(you will need correctly configured schroots with union overlay
for this)
Sorry for the long mail, I tried to be clear rather than short. If something is not clear nonetheless, please ask.
Best, AymericP.S. @Sean, my fork does not include your latest commits on deb/emacs/d/sid/master. I do not think that matters, we never use emacs-pgtk for any of our builds, as far as I can tell.
P.P.S I think my build environment is pretty much equivalent to the one on buildd, so if, in the result file, you have digits different than 0 after some source package you maintain, you will probably receive a new FTBFS bug soon :).
Attachment:
paquets_debian_emacsen
Description: Binary data
#!/bin/sh
# Attempts to build the package provided as argument
# First only with the archive
# Second with the .deb files in the directory pointed by variable DEB_EXTRA_PACKAGES
# And compare the results ; on the standard output is written :
# - the name of the source package
# - the return value of the first attempt
# - the return value of the second attempt
if test $# -lt 1; then
echo "Missing argument : source package"
exit 1
fi
# The directory holding the modified versions of emacs, set to a correct value
DEB_EXTRA_PACKAGES=/home/rico/temp/emacs
TEMP_DIRECTORY=$(mktemp -d)
cd $TEMP_DIRECTORY
apt source -t sid --only-source --download-only $1 1>/dev/null 2>/dev/null
dscfile=$(find . -name "*.dsc")
if test "$dscfile" = ""; then
echo "No such package : $1"
exit 1
fi
# First attempt
sbuild -d unstable --chroot=unstable-amd64-sbuild --no-run-lintian --no-run-piuparts $dscfile 1>/dev/null 2>/dev/null
first_attempt="$?"
# Second attempt
sbuild -d unstable --chroot=unstable-amd64-sbuild --no-run-lintian --no-run-piuparts --extra-package=$DEB_EXTRA_PACKAGES $dscfile 1>/dev/null 2>/dev/null
second_attempt="$?"
echo "$1 $first_attempt $second_attempt"
cd
rm -rf $TEMP_DIRECTORY
Attachment:
emacs_results3
Description: Binary data