Rename spkg-build, spkg-install etc. templates as spkg-build.in, spkg-install.in etc.
mkoeppe opened this issue · 37 comments
It is unnecessarily confusing that build/bin/sage-spkg transforms the template spkg-install to an executable script that is also called spkg-install.
This ticket renames all build/pkgs/SPKG/spkg-* of packages other than type=script (which are regular executable shell scripts already) to build/pkgs/SPKG/spkg-*.in.
In addition to the increased clarity, this is preparation for #29122 (Build packages in build/pkgs/SPKG/src, not SAGE_LOCAL/var/tmp/sage/build/SPKG-...).
CC: @embray @dimpase @jhpalmieri
Component: build
Author: Matthias Koeppe
Branch/Commit: ec540dd
Reviewer: Dima Pasechnik, John Palmieri
Issue created by migration from https://trac.sagemath.org/ticket/29096
Commit: 4524b04
Branch pushed to git repo; I updated commit sha1. New commits:
4524b04 | Update developer manual |
Author: Matthias Koeppe
You need to at least make this change:
diff --git a/build/bin/sage-spkg b/build/bin/sage-spkg
index 66ff1d08bb..4776654a4d 100755
--- a/build/bin/sage-spkg
+++ b/build/bin/sage-spkg
@@ -770,7 +770,7 @@ for script in $WRAPPED_SCRIPTS; do
script="spkg-$script"
- if [ -f "$script" ]; then
+ if [ -f "$script.in" ]; then
if [ "$USE_LOCAL_SCRIPTS" = "yes" ]; then
if [ -x "$script.in" ]; then
msg="$script.in should not be marked executable in the build/pkgs directory"Also, you shouldn't rename elliptic_curves/spkg-install.py.
indeed, as far as I know, none of build/pkgs/*/*.py are templates, they are Python scripts, and should remain so.
Thanks for catching this -- query-replace-regexp was a little bit too enthusiastic
Thanks for the quick reactions. Here's a version that actually has a chance of working
looks good to me.
Reviewer: Dima Pasechnik
Thanks!
I think this is a rather disruptive change that doesn't seem well-motivated. What problem is this fixing?
Improper file naming. Templates for scripts should be easily distinguishable from non-templates, i.e. "real" scripts. What's so disruptive about it? It's a bit more sanity in a quite complicated setup.
Actually, this confuses me, because the .in extension tends to suggest that the real files are generated by a configure script.
Also, this is only a problem if the generated files live side-by-side with the templates in the source. The generated files in this case are only created in temporary build directories. It doesn't actually disrupt anything.
Again, what real problem is this solving?
This seems to make the instructions for creating SPKGs more confusing as well, and adds disruption in the process that people are already familiar with for now clear gain. Please don't set tickets to positive review if there is a reasonable disagreement here.
Also disruptive: Mass file rename which makes any pending tickets updating SPKGs invalid. Might also break other existing scripts, etc. Unless there is a serious problem this is solving it seems way overboard.
Replying to @dimpase:
Improper file naming. Templates for scripts should be easily distinguishable from non-templates, i.e. "real" scripts.
They are already distinguished by the fact that they are not marked executable. If you really want to make a distinction, you can have the files output by sage-spkg have a different name, but that's not much clearer. It's nice when debugging a build directory to have the output spkg-install just work.
Replying to @dimpase:
Improper file naming. Templates for scripts should be easily distinguishable from non-templates,
In fact, come to think of it, these files are not even templates. The template is the code in sage-spkg:
write_script_wrapper() {
local script="$1"
local script_dir="$2"
trap "echo >&2 Error: Unexpected error writing wrapper script for $script; exit \$_" ERR
if head -1 "$script" | grep '^#!.*$' >/dev/null; then
echo >&2 "Error: ${script##*/} should not contain a shebang line; it will be prepended automatically."
exit 1
fi
local tmpscript="$(dirname "$script")/.tmp-${script##*/}"
cat > "$tmpscript" <<__EOF__
#!/usr/bin/env bash
export SAGE_ROOT="$SAGE_ROOT"
export SAGE_SRC="$SAGE_SRC"
export SAGE_PKG_DIR="$script_dir"
export SAGE_SPKG_SCRIPTS="$SAGE_SPKG_SCRIPTS"
export PKG_NAME="$PKG_NAME"
export PKG_BASE="$PKG_BASE"
export PKG_VER="$PKG_VER"
for lib in "\$SAGE_ROOT/build/bin/sage-dist-helpers" "\$SAGE_SRC/bin/sage-env" "\$SAGE_ROOT/build/bin/sage-build-env-config"; do
source "\$lib"
if [ \$? -ne 0 ]; then
echo >&2 "Error: failed to source \$lib"
echo >&2 "Is \$SAGE_ROOT the correct SAGE_ROOT?"
exit 1
fi
done
sdh_guard
if [ \$? -ne 0 ]; then
echo >&2 "Error: sdh_guard not found; Sage environment was not set up properly"
exit 1
fi
cd "\$SAGE_PKG_DIR"
if [ \$? -ne 0 ]; then
echo >&2 "Error: could not cd to the package build directory \$SAGE_PKG_DIR"
exit 1
fi
__EOF__
cat "$script" >> "$tmpscript"
mv "$tmpscript" "$script"
chmod +x "$script"
trap - ERR
}
That's the template. The files in build/pkg/*/spkg-* are not templates. They are snippets that are interpolated into that template.
OK, they are template parameters, if you prefer, that's a better description indeed. What's important is that they are not scripts by themselves.
Right. And that's clear from the lack of shebang line or executable flags.
If you really want to make a clearer distinction, one idea might be to have the working scripts output by sage-spkg have a .sh extension or something. I don't think it's necessary, but if there is a legitimate confusion that it might clear up I'm for it.
The files could just as well be generated by configure. The only thing that's missing for that is a line
@SPKG_SCRIPT@
on top of each of the .in files. I'm not proposing to do that in this ticket. But I hope this explains why the files now have the .in extension.
Replying to @embray:
Right. And that's clear from the lack of shebang line or executable flags.
If you really want to make a clearer distinction, one idea might be to have the working scripts output by sage-spkg have a
.shextension or something. I don't think it's necessary, but if there is a legitimate confusion that it might clear up I'm for it.
there are build/pkgs/*/spkg-install files which are proper shell scripts (for spkgs which are of type script).
One should make them namewise-distinct from templates, e.g. as done on this branch, or by appending .sh to them.
I think the name changes are justified.
It would be less invasive but accomplish the same thing (wouldn't it?) if we kept the spkg-install files as they are, but had sage-spkg create new scripts spkg-install.sh in the build directories.
Would that make everyone happy? Or at least be a reasonable compromise?
as I explained in comment:23, some of these files are scripts, and some (most) are not.
The invasive change happened when most of these files stopped being scripts, so this ticket merely sets the record straight.
Replying to @embray:
Also disruptive: Mass file rename which makes any pending tickets updating SPKGs invalid.
Any merge conflicts of this kind are trivial to resolve.
Might also break other existing scripts, etc.
This is unsubstantiated.
Replying to @embray:
Replying to @dimpase:
Improper file naming. Templates for scripts should be easily distinguishable from non-templates,
In fact, come to think of it, these files are not even templates.
If you have a better word for this, I'll be happy to change the documentation.
Autoconf just calls these files "input files" (hence .in)
Also there aren't many open tickets changing these files, so that's very minor problem, if at all. Actually while we are at it I would go further and drop these spkg- and package- prefixes everywhere in build/pkgs/*/, too.
Description changed:
---
+++
@@ -2,3 +2,4 @@
This ticket renames all `build/pkgs/SPKG/spkg-*` of packages other than `type=script` (which are regular executable shell scripts already) to `build/pkgs/SPKG/spkg-*.in`.
+In addition to the increased clarity, this is preparation for #29122 (Build packages in `build/pkgs/SPKG/src`, not `SAGE_LOCAL/var/tmp/sage/build/SPKG-...`).Replying to @dimpase:
as I explained in comment:23, some of these files are scripts, and some (most) are not.
The invasive change happened when most of these files stopped being scripts, so this ticket merely sets the record straight.
You are using the word "invasive" differently than the rest of us, I think. I just mean: it touches a lot of files.
Anyway, it's nice to see everyone so entrenched in their positions.
Changed reviewer from Dima Pasechnik to Dima Pasechnik, John Palmieri
Nothing to merge