Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
slel opened this issue Mar 20, 2021 · 25 comments
Closed

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

slel opened this issue Mar 20, 2021 · 25 comments

Comments

@slel
Copy link
Member

slel commented Mar 20, 2021

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.

CC: @dimpase @embray @orlitzky @mkoeppe @slel

Component: build: configure

Author: Frédéric Chapoton, Michael Orlitzky

Branch/Commit: f9e58da

Reviewer: Michael Orlitzky, Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/31528

@slel slel added this to the sage-9.3 milestone Mar 20, 2021
@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 24, 2021

comment:1

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.

@mkoeppe mkoeppe modified the milestones: sage-9.3, sage-9.4 Mar 24, 2021
@slel
Copy link
Member Author

slel commented May 11, 2021

comment:2

Previous related tickets:

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

@slel

This comment has been minimized.

@mkoeppe mkoeppe modified the milestones: sage-9.4, sage-9.5 Jul 19, 2021
@fchapoton
Copy link
Contributor

comment:4

here is a branch


New commits:

30988c7no tab in spkg-configure

@fchapoton
Copy link
Contributor

Commit: 30988c7

@fchapoton
Copy link
Contributor

Branch: u/chapoton/31528

@fchapoton
Copy link
Contributor

Author: Frédéric Chapoton

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2021

Changed commit from 30988c7 to 2de538d

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2021

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

2de538dtrac 32401 change one doctest for acosh

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2021

Changed commit from 2de538d to 30988c7

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Aug 21, 2021

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

30988c7no tab in spkg-configure

@slel
Copy link
Member Author

slel commented Aug 21, 2021

comment:7

Thanks Frédéric for this branch.

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

@orlitzky
Copy link
Contributor

comment:8

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?

@fchapoton
Copy link
Contributor

comment:9

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.

@orlitzky
Copy link
Contributor

comment:10

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.

@orlitzky
Copy link
Contributor

Changed branch from u/chapoton/31528 to u/mjo/ticket/31528

@orlitzky
Copy link
Contributor

Changed author from Frédéric Chapoton to Frédéric Chapoton, Michael Orlitzky

@orlitzky
Copy link
Contributor

Reviewer: Michael Orlitzky

@orlitzky
Copy link
Contributor

Changed commit from 30988c7 to f9e58da

@slel
Copy link
Member Author

slel commented Aug 21, 2021

comment:11

Replying to @orlitzky:

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?

@orlitzky
Copy link
Contributor

comment:12

Replying to @slel:

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.

@dimpase
Copy link
Member

dimpase commented Aug 25, 2021

Changed reviewer from Michael Orlitzky to Michael Orlitzky, Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Aug 25, 2021

comment:13

lgtm

@slel
Copy link
Member Author

slel commented Aug 28, 2021

comment:14

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

@vbraun
Copy link
Member

vbraun commented Aug 31, 2021

Changed branch from u/mjo/ticket/31528 to f9e58da

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants