#29096 closed enhancement (fixed)

Rename spkg-build, spkg-install etc. templates as spkg-build.in, spkg-install.in etc.

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.1
Component: build Keywords:
Cc: embray, dimpase, jhpalmieri Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik, John Palmieri
Report Upstream: N/A Work issues:
Branch: ec540dd (Commits, GitHub, GitLab) Commit: ec540dd345a3127ef19bdb2def38e63cdb6a3361
Dependencies: Stopgaps:

Status badges

Description (last modified by mkoeppe)

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-...).

Change History (37)

comment:1 Changed 16 months ago by mkoeppe

  • Branch set to u/mkoeppe/rename_spkg_build__spkg_install_etc__templates_as_spkg_build_in__spkg_install_in_etc_

comment:2 Changed 16 months ago by git

  • Commit set to 4524b04f9ecff7346aec7d27caf985629f8ca7e8

Branch pushed to git repo; I updated commit sha1. New commits:

4524b04Update developer manual

comment:3 Changed 16 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Cc jhpalmieri paulmasson added
  • Status changed from new to needs_review

comment:4 Changed 16 months ago by jhpalmieri

You need to at least make this change:

  • build/bin/sage-spkg

    diff --git a/build/bin/sage-spkg b/build/bin/sage-spkg
    index 66ff1d08bb..4776654a4d 100755
    a b for script in $WRAPPED_SCRIPTS; do 
    770770
    771771    script="spkg-$script"
    772772
    773     if [ -f "$script" ]; then
     773    if [ -f "$script.in" ]; then
    774774        if [ "$USE_LOCAL_SCRIPTS" = "yes" ]; then
    775775            if [ -x "$script.in" ]; then
    776776                msg="$script.in should not be marked executable in the build/pkgs directory"

comment:5 Changed 16 months ago by jhpalmieri

Also, you shouldn't rename elliptic_curves/spkg-install.py.

comment:6 Changed 16 months ago by dimpase

indeed, as far as I know, none of build/pkgs/*/*.py are templates, they are Python scripts, and should remain so.

comment:7 Changed 16 months ago by mkoeppe

Thanks for catching this -- query-replace-regexp was a little bit too enthusiastic

comment:8 Changed 16 months ago by git

  • Commit changed from 4524b04f9ecff7346aec7d27caf985629f8ca7e8 to ec540dd345a3127ef19bdb2def38e63cdb6a3361

Branch pushed to git repo; I updated commit sha1. New commits:

2724083Undo rename of build/pkgs/*/spkg-install.py
ec540ddFix up sage-spkg

comment:9 Changed 16 months ago by mkoeppe

Thanks for the quick reactions. Here's a version that actually has a chance of working

comment:10 Changed 16 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

looks good to me.

comment:11 Changed 16 months ago by mkoeppe

Thanks!

comment:12 Changed 16 months ago by embray

  • Status changed from positive_review to needs_info

I think this is a rather disruptive change that doesn't seem well-motivated. What problem is this fixing?

comment:13 follow-ups: Changed 16 months ago by dimpase

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.

comment:14 Changed 16 months ago by dimpase

  • Status changed from needs_info to positive_review

comment:15 Changed 16 months ago by embray

  • Status changed from positive_review to needs_info

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?

comment:16 Changed 16 months ago by embray

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.

comment:17 follow-up: Changed 16 months ago by embray

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.

comment:18 in reply to: ↑ 13 Changed 16 months ago by embray

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.

comment:19 in reply to: ↑ 13 ; follow-up: Changed 16 months ago by 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. 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.

comment:20 Changed 16 months ago by dimpase

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.

comment:21 follow-up: Changed 16 months ago by 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 .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.

comment:22 Changed 16 months ago by mkoeppe

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.

comment:23 in reply to: ↑ 21 Changed 16 months ago by dimpase

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 .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.

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.

comment:24 Changed 16 months ago by paulmasson

  • Cc paulmasson removed

comment:25 Changed 16 months ago by dimpase

  • Status changed from needs_info to positive_review

I think the name changes are justified.

comment:26 Changed 16 months ago by jhpalmieri

  • Status changed from positive_review to needs_info

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?

comment:27 follow-up: Changed 16 months ago by 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.

comment:28 in reply to: ↑ 17 Changed 16 months ago by mkoeppe

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.

comment:29 in reply to: ↑ 19 Changed 16 months ago by mkoeppe

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)

comment:30 follow-up: Changed 16 months ago by dimpase

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.

comment:31 in reply to: ↑ 30 Changed 16 months ago by mkoeppe

Replying to dimpase:

Actually while we are at it I would go further and drop these spkg- and package- prefixes everywhere in build/pkgs/*/, too.

This will only be possible after #29289 (Remove support for old-style SPKGs). So let's not do this in this ticket.

comment:32 Changed 16 months ago by mkoeppe

  • Description modified (diff)

comment:33 in reply to: ↑ 27 Changed 16 months ago by jhpalmieri

  • Status changed from needs_info to positive_review

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.

comment:34 Changed 16 months ago by mkoeppe

  • Reviewers changed from Dima Pasechnik to Dima Pasechnik, John Palmieri

comment:35 Changed 15 months ago by vbraun

  • Status changed from positive_review to needs_info

Nothing to merge

comment:36 Changed 15 months ago by vbraun

  • Status changed from needs_info to positive_review

comment:37 Changed 15 months ago by vbraun

  • Branch changed from u/mkoeppe/rename_spkg_build__spkg_install_etc__templates_as_spkg_build_in__spkg_install_in_etc_ to ec540dd345a3127ef19bdb2def38e63cdb6a3361
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.