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

Upgrade autotools & cleanup #1

Merged
merged 34 commits into from
Apr 27, 2018
Merged

Conversation

saraedum
Copy link
Member

@saraedum saraedum commented Apr 23, 2018

This tries to modernize the use of autotools in cddlib (see also https://groups.google.com/forum/#!msg/sage-devel/uHsTOd5sTxY/ivpNHT1MAwAJ)

Most significant changes:

  • Drop all (?) autogenerated files from the repository.
  • Add ./bootstrap script to regenerate files used by autotools and friends.
  • Make make distcheck work again which creates cddlib-0.94i.tar.gz for distribution. (So in the future to create a releasable tarball one would increase the version number in configure.ac, run ./bootstrap, run ./configure and make distcheck.)

Many of the changes are originally due to @embray.

TODO

  • Check that this works for OSX Ok. TravisCI takes care of that.
  • Update the README so it talks about ./bootstrap for people who clone from here instead of downloading a tarball. Let's postpone this to a separate PR. The README needs some fixes to make it looks nice on github anyway.
  • Check the contents of the tarball (see below)
  • Things such as export gmpdir := /usr/local in Makefile.am seem dubious. ./configure should decide that I guess? I just set -lgmp conditionally now. I think that's good enough for GMP (otherwise we could use pkg-config?) Note that this is also what most distributions patch into the configure.in file.
  • cddlib installs itself as libcdd.so.0.0.0 and libcddgmp.so.0.0.0. This can now be changed in configure.ac.
  • should the binaries really install to /usr/bin/? I do not really understand what people use in the end, but maybe some things should not be on the PATH? (So maybe these should go into /usr/libexec instead? Or not be installed at all?) Let's put everything into PREFIX/bin for now.
  • documentation does not install with make install. Should it? It does now.
  • should the documentation be built as part of make dist? the currently distributed .pdf, .dvi, .ps is now built by make dist.
  • examples do not install with make install. Should they? They do now.
  • Have a look at Debian's patches.
  • Have a look at Archlinux's patches.
  • Have a look at SageMath's patches.
  • Have a look at Conda's patches.

@saraedum saraedum added the enhancement New feature or request label Apr 23, 2018
@saraedum
Copy link
Member Author

saraedum commented Apr 23, 2018

TODO

These files are part of the repository but not part of the tarball. Check which of the are actually missing:

  • bootstrap. Ok. Now part of the tarball.
  • doc. Ok. We now ship the pdf, ps, dvi, but not the html documentation (like before). Is that Ok?
  • .git, Ok, the git history should not be in the tarball.
  • .gitignore, Ok, nothing git related should be in the tarball.
  • HISTORY, not a standard file and identical to NEWS, so I think it's Ok if it's missing.
  • README.core2processor, it's included again.
  • .gitignore, Ok, nothing git related should be in the tarball.
  • src/minkowski.c, included
  • src/redexter.c, included
  • src/redundancies.c, included
  • src/redundancies_clarkson.c, included
  • src/testuniq.c, included
  • src-gmp/.gitignore, Ok, nothing git related should be in the tarball.

These files are part of the tarball but not in the repository. That's ok. They are all generated by autotools.

  • aclocal.m4
  • compile
  • config.guess
  • config.sub
  • configure
  • depcomp
  • INSTALL
  • install-sh
  • Makefile.in
  • lib-src-gmp/Makefile.in
  • ltmain.sh
  • m4
  • Makefile.in
  • missing
  • src/Makefile.in
  • src-gmp/Makefile.in

Looking for "main" is cheating a bit (main is always there.) Instead we look
for __gmpz_init as proposed by GMP. If GMP can not be found, we don't build the
GMP enabled parts.

Also this drops obsolete checks (according to autoconf's manual) such as
AC_HEADER_STDC, AC_C_CONST (check that "const" is supported).

cddlib.so is now in theory versioned. We probably want to stick to 0.0.0 as
some people might rely on this?

Finally, this drops a bunch of hackish variables in the .am files around GMP.
@saraedum
Copy link
Member Author

I am not sure what to do with these source files:

  • src/minkowski.c
  • src/redexter.c
  • src/redundancies.c
  • src/redundancies_clarkson.c
  • src/testuniq.c
  • src-gmp/redundancies.c
  • src-gmp/redundancies_clarkson.c
  • src-gmp/testuniq.c

They are apparently not used in any of the Makefiles. Is that on purpose or should they be built as well?

@saraedum saraedum mentioned this pull request Apr 24, 2018
@saraedum
Copy link
Member Author

saraedum commented Apr 24, 2018

I had a look at all the patches used elsewhere.

  • I create Include cdd_both_reps #3 to add an additional binary that everybody is using apparently (probably because Sage is using it.)
  • SageMath adds a "portable random number generator". I guess the reason is to have the randomness of results not be dependent of the underlying implementation of rand. I can see why we do that at SageMath, but I think it's a bad idea anyway. If stuff is random, we should just live with that in our doctests. (Or we should link a patched libc that gives us portable random numbers…but we shouldn't patch this in a single package.); this is now https://trac.sagemath.org/ticket/25238
  • Everybody is patching the GMP detection (which I did here as well.)
  • Debian has a man-page for the additional binary but that's probably not worth it for just one binary.

@saraedum
Copy link
Member Author

The author answered about the .c file that are not in the Makefiles:

Those .c files not used are experimental programs that may be
of interest for researchers and advanced users. They are there so that
anyone who has advanced questions can see how some advanced techniques
in computational geometry might test the behaviour of certain algorithms
that might be interesting. I do not know these examples should be in src directories.

So, I think it makes sense to build them, so the interested developer does not need to figure out how to do that. But they should not be installed.

@saraedum
Copy link
Member Author

saraedum commented Apr 24, 2018

The author answered about the examples:

I do not know what is the usual practice. If the user expects that these
example files are in a certain directory, they should be copied there.
But, I do not like these files are copied to the primary locations under /usr .
I much prefer all third party softwares are in /usr/local, because one can easily
separate the core binaries/libraries from the third party ones.

Since the documentation mentions these example files, they should probably be part of the documentation? Whether they get installed to /usr or /usr/local depends on how you call ./configure. The default is indeed for everything to go to /usr/local. But if you install this with say Debian, then they would of course go into /usr.

@saraedum
Copy link
Member Author

@komeifukuda dc05ae0 makes your sed script unnecessary I think.

@saraedum
Copy link
Member Author

@komeifukuda strangely, testuniq.c in src and src-gmp are not the same. Here is the diff.

--- src/testuniq.c	2018-04-22 22:25:01.636505790 +0200
+++ src-gmp/testuniq.c	2018-04-22 22:25:01.635505776 +0200
@@ -59,17 +59,15 @@
 
 int main(int argc, char *argv[])
 {
-  dd_MatrixPtr M=NULL,M1=NULL,M2=NULL;
+  dd_MatrixPtr M=NULL,M1=NULL,M2=NULL,M3=NULL,M4=NULL;
   dd_rowrange i;
   dd_colrange d;
   dd_ErrorType err=dd_NoError;
-  dd_rowset redrows,linrows,posset;
-  dd_rowindex newpos=NULL, newpos1=NULL, newpos2=NULL;
+  dd_rowset redrows,linrows;
+  dd_rowindex newpos1=NULL, newpos2=NULL,newpos4=NULL;
   mytype val;
   dd_DataFileType inputfile;
   FILE *reading=NULL;
-  int foi;
-  dd_Arow certificate;
 
   dd_set_global_constants();  /* First, this must be called. */
 
@@ -92,69 +90,67 @@
 
   d=M->colsize;
   
-  /* 
   printf("\nInput Matrix.\n");
-  dd_WriteMatrix(stdout, M); 
-  */
- 
-  M1=dd_MatrixSortedUniqueCopy(M,&newpos1);
-  printf("\nSort and remove duplicates with dd_MatrixSortedUniqueCopy.\n");
+  dd_WriteMatrix(stdout, M);
+  M1=dd_MatrixSortedCopy(M,&newpos1);
+  printf("\nNormalize and sort the matrix with dd_MatrixSortedCopy.\n");
   printf(" Row index changes -- original:new\n");
   for (i=1;i<=M->rowsize; i++){
     printf(" %ld:%ld",i,newpos1[i]);
   }
   printf("\n");
-  /* dd_WriteMatrix(stdout, M1); */
 
-  dd_InitializeArow(M1->colsize+2, &certificate);
-  fprintf(stdout, "\nCheck whether the system contains an implicit linearity.\n");
-  foi=dd_FreeOfImplicitLinearity(M1, certificate, &posset, &err);
-  switch (foi) {
-    case 1:
-         fprintf(stdout, "  It is free of implicit linearity.\n");
-      break;
-      
-    case 0:
-         fprintf(stdout, "  It is not free of implicit linearity.\n");
-      break;
-
-    case -1:
-         fprintf(stdout, "  The input system is trivial (i.e. the empty H-polytope or the V-rep of the whole space.\n");
-      break;
+  dd_WriteMatrix(stdout, M1);
+  
+  M2=dd_MatrixUniqueCopy(M1,&newpos2);
+  printf("\nRemove row (consecutive) duplicates with dd_MatrixUniqueCopy.\n");
+  printf(" Row index changes -- original:new\n");
+  for (i=1;i<=M1->rowsize; i++){
+    printf(" %ld:%ld",i,newpos2[i]);
+  }
+  printf("\n");
+  dd_WriteMatrix(stdout, M2);
+   
+   
+  M4=dd_MatrixSortedUniqueCopy(M,&newpos4);
+  printf("\nTwo operations can be done at once with dd_MatrixSortedUniqueCopy.\n");
+  printf(" Row index changes -- original:new\n");
+  for (i=1;i<=M->rowsize; i++){
+    printf(" %ld:%ld",i,newpos4[i]);
   }
+  printf("\n");
+  dd_WriteMatrix(stdout, M4);
 
 
-  fprintf(stdout, "\nOne can then remove nontrivial redundant rows with dd_RedundantRows.\n Redundant rows:\n");
-  redrows=dd_RedundantRowsWithPresorting(M1, &err);
-  /* redrows=dd_RedundantRows(M1, &err);  */
+  fprintf(stdout, "\nOne can then remove nontrivial redundant rows with dd_RedundantRows.\n Redundant rows:");
+  redrows=dd_RedundantRows(M2, &err);
   set_fwrite(stdout, redrows);
-  printf("\n");
 
-  M2=dd_MatrixSubmatrix2(M1, redrows, &newpos);
+  M3=dd_MatrixSubmatrix(M2, redrows);
   dd_FreeMatrix(M1);
+  dd_FreeMatrix(M2);
   set_free(redrows);
-  free(newpos);
-  
+
   fprintf(stdout, " Implicit linearity (after removal of redundant rows): ");
-  linrows=dd_ImplicitLinearityRows(M2, &err);
+  linrows=dd_ImplicitLinearityRows(M3, &err);
 
   if (M->representation==dd_Generator)
     fprintf(stdout," %ld  ", set_card(linrows));
   else 
     fprintf(stdout," %ld  ", set_card(linrows));
   set_fwrite(stdout,linrows);
-  set_uni(M2->linset, M2->linset, linrows); 
+  set_uni(M3->linset, M3->linset, linrows); 
     /* add the implicit linrows to the given linearity rows */
 
   printf("\nNonredundant representation (except for the linearity part):\n");
-  dd_WriteMatrix(stdout, M2);
-  dd_WriteLPStats(stdout);
+  dd_WriteMatrix(stdout, M3);
   set_free(linrows);
 
   dd_FreeMatrix(M);
-  dd_FreeMatrix(M2);
+  dd_FreeMatrix(M3);
+  dd_FreeMatrix(M4);
   dd_clear(val);
-  free(newpos1); free(newpos2);
+  free(newpos1); free(newpos2); free(newpos4);
 
 _L99:;
   /* if (err!=dd_NoError) dd_WriteErrorMessages(stderr,err); */

@saraedum
Copy link
Member Author

saraedum commented Apr 24, 2018

@komeifukuda Is there any reason why one would want to build the binaries without GMP support if GMP is available? I think it's a bit non-standard to build two flavours of every binary.

@saraedum
Copy link
Member Author

saraedum commented Apr 24, 2018

Or rather: I think if one really wants this, then it should simply be:

./configure --without-gmp
make
./configure --program_suffix=_gmp
make

@saraedum
Copy link
Member Author

So. What is the right default: with or without GMP support? @komeifukuda

@saraedum saraedum force-pushed the autotool-cleanup branch 2 times, most recently from 4c2f282 to c7f731b Compare April 24, 2018 23:18
so they should link against the headers in the paths provided by -I and not
assume that the headers are available locally.
it worked locally because I do have cddlib installed
by putting pdflatex on the PATH
@saraedum
Copy link
Member Author

@komeifukuda I think I have fixed all the things that I wanted to. If you are happy with this this, just press "Merge pull request". Otherwise I am looking forward to hearing your suggestions.

@saraedum
Copy link
Member Author

If you want to try this out:

git clone git@github.com:saraedum/cddlib.git -b autotool-cleanup
cd cddlib
./bootstrap
./configure
make dist

Komei Fukuda wrote:
> By the way, among the .c files in src directory not used, minkowski.c and
> testuniq.c should be removed.
> They and not complete and not meant to be distributed.
@saraedum saraedum merged commit 88a5c89 into cddlib:master Apr 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants