Opened 14 months ago

Closed 9 months ago

#31528 closed enhancement (fixed)

Cleanup spkg-configure.m4 files that mix tabs and spaces

Reported by: slelievre Owned by:
Priority: critical Milestone: sage-9.5
Component: build: configure Keywords:
Cc: dimpase, embray, mjo, mkoeppe, slelievre Merged in:
Authors: Frédéric Chapoton, Michael Orlitzky Reviewers: Michael Orlitzky, Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: f9e58da (Commits, GitHub, GitLab) Commit: f9e58dab2a38012cb0d684b63d5c7934b94963b5
Dependencies: Stopgaps:

Status badges

Description (last modified by slelievre)

The spkg-configure files for the following packages mix tabs and spaces: arb brial cmake flint gcc gfortran git gmp iml ninja_build pari patch pcre ppl python3 symmetrica.

This ticket is to make them use spaces only.

Note however discussion at #13899 warns against changing whitespace in a lot of files at once.

Change History (15)

comment:1 Changed 14 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Sage development has entered the release candidate phase for 9.3. Setting a new milestone for this ticket based on a cursory review of ticket status, priority, and last modification date.

comment:2 Changed 12 months ago by slelievre

  • Description modified (diff)

Previous related tickets:

  • #15436: Cksum uses tabs instead of spaces, breaking sage-spkg's regex
  • #13899: Don't use TAB characters for indentation
  • #13146: Removing tabs in .rst, .tex and .pxi files

This shell command spots files with tabs when run from the Sage root folder:

$ git grep $'\t' . | cut -f 1 -d ":" | uniq \
  | grep -v Make | grep -v -e '^Binary' | grep -v patches

In Sage 9.3 it reveals:

bootstrap
build/bin/write-dockerfile.sh
build/pkgs/_prereq/distros/freebsd.txt
build/pkgs/arb/spkg-configure.m4
build/pkgs/brial/spkg-configure.m4
build/pkgs/bzip2/spkg-configure.m4
build/pkgs/cmake/spkg-configure.m4
build/pkgs/cvxopt/spkg-install.in
build/pkgs/ecm/spkg-install.in
build/pkgs/flint/spkg-configure.m4
build/pkgs/gcc/spkg-configure.m4
build/pkgs/gfortran/spkg-configure.m4
build/pkgs/git/spkg-configure.m4
build/pkgs/gmp/spkg-configure.m4
build/pkgs/iml/spkg-configure.m4
build/pkgs/ninja_build/spkg-configure.m4
build/pkgs/pari/spkg-configure.m4
build/pkgs/patch/spkg-configure.m4
build/pkgs/pcre/spkg-configure.m4
build/pkgs/ppl/spkg-configure.m4
build/pkgs/python3/spkg-configure.m4
build/pkgs/symmetrica/spkg-configure.m4
m4/ax_absolute_header.m4
m4/ax_compiler_vendor.m4
m4/ax_gcc_option.m4
m4/ax_gxx_option.m4
m4/ax_prog_perl_modules.m4
src/.relint.yml
src/doc/en/developer/portability_testing.rst
src/doc/en/reference/parallel/media/map_reduce_arch.fig
src/sage/ext_data/graphs/graph_plot_js.html
src/sage/ext_data/magma/latex/latex.m
src/sage/ext_data/pari/simon/ell.gp
src/sage/graphs/cliquer/cl.c
src/sage/groups/perm_gps/partn_ref2/refinement_generic.h
src/sage/modular/arithgroup/farey.cpp
src/sage/modular/arithgroup/farey.hpp
src/sage/modular/arithgroup/sl2z.hpp
src/sage/rings/polynomial/weil/power_sums.c
src/sage/rings/polynomial/weil/power_sums.h

comment:3 Changed 10 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

comment:4 Changed 9 months ago by chapoton

  • Authors set to Frédéric Chapoton
  • Branch set to u/chapoton/31528
  • Commit set to 30988c777a5492aecf5198463aabe4a33b5bb602
  • Status changed from new to needs_review

here is a branch


New commits:

30988c7no tab in spkg-configure

comment:5 Changed 9 months ago by git

  • Commit changed from 30988c777a5492aecf5198463aabe4a33b5bb602 to 2de538d920229e2c098cd1cbc7db1d54362d1348

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

2de538dtrac 32401 change one doctest for acosh

comment:6 Changed 9 months ago by git

  • Commit changed from 2de538d920229e2c098cd1cbc7db1d54362d1348 to 30988c777a5492aecf5198463aabe4a33b5bb602

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

30988c7no tab in spkg-configure

comment:7 Changed 9 months ago by slelievre

  • Cc dimpase embray mjo mkoeppe added

Thanks Frédéric for this branch.

Would someone who knows about spkg-configure.m4 files give the green light on this?

comment:8 follow-up: Changed 9 months ago by mjo

The m4 and C-language changes all look fine to me at first glance. I'm probably responsible for a few of the tabs in spkg-configure.m4, but I do indeed prefer spaces when I'm paying attention.

Are magma and GP whitespace-insensitive too?

comment:9 in reply to: ↑ 8 Changed 9 months ago by chapoton

Are magma and GP whitespace-insensitive too?

I think that gp does not care. No idea about magma, but it certainly does not require TABs.

comment:10 follow-up: Changed 9 months ago by mjo

  • Authors changed from Frédéric Chapoton to Frédéric Chapoton, Michael Orlitzky
  • Branch changed from u/chapoton/31528 to u/mjo/ticket/31528
  • Commit changed from 30988c777a5492aecf5198463aabe4a33b5bb602 to f9e58dab2a38012cb0d684b63d5c7934b94963b5
  • Reviewers set to Michael Orlitzky

I took a closer look and added two commits to fix the indentation where it was weird. There are still others, but I don't have time to go through and fix them all. They're not new issues.

The GP change is OK too. I have no way to run the magma code, but since there are already spaces in that file, I guess it's unlikely that changing some existing tabs into more spaces will cause a problem.

If someone can double-check my two new commits (which should contain only whitespace changes), please set to positive review after.

comment:11 in reply to: ↑ 10 ; follow-up: Changed 9 months ago by slelievre

Replying to mjo:

If someone can double-check my two new commits (which should contain only whitespace changes), please set to positive review after.

In build/pkgs/iml/spkg-configure.m4, besides fixing indentation, you remove two lines.

Is that intended?

comment:12 in reply to: ↑ 11 Changed 9 months ago by mjo

Replying to slelievre:

In build/pkgs/iml/spkg-configure.m4, besides fixing indentation, you remove two lines.

Is that intended?

The "dnl" lines? Yeah, those were comments, believe it or not.

comment:13 Changed 9 months ago by dimpase

  • Reviewers changed from Michael Orlitzky to Michael Orlitzky, Dima Pasechnik
  • Status changed from needs_review to positive_review

lgtm

comment:14 Changed 9 months ago by slelievre

  • Priority changed from major to critical

Setting to critical as this conditions work on spkg-configure files in other tickets.

comment:15 Changed 9 months ago by vbraun

  • Branch changed from u/mjo/ticket/31528 to f9e58dab2a38012cb0d684b63d5c7934b94963b5
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.