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

Update cddlib's autotooling #21952

Closed
embray opened this issue Nov 23, 2016 · 32 comments
Closed

Update cddlib's autotooling #21952

embray opened this issue Nov 23, 2016 · 32 comments

Comments

@embray
Copy link
Contributor

embray commented Nov 23, 2016

As discussed in this thread, cddlib's autotools files are quite outdated and in due for modernization. In fact, somehow, its configure.in is not even compatible with the version of automake that the Makefile.ins were generated with. This makes it difficult/impossible to make update cddlib's Makefiles (see #15872).

I reworked the latest version of cddlib (0.94h, though this work would also apply to at least the previous version, 0.94g) to use automake 1.14.1 (currently the highest version in sage, though #21196 would update that to 1.15).

I also cleaned up the contents of the tarball (removed .DS_Store files and other crud), and made it so that make distcheck works.

I've attached an updated upstream tarball, as well as a set of patches to the original upstream that should reproduce it.

Upstream: Reported upstream. Developers acknowledge bug.

Component: packages: standard

Keywords: cddlib autotools

Author: Erik Bray

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

@embray embray added this to the sage-7.5 milestone Nov 23, 2016
@embray
Copy link
Contributor Author

embray commented Nov 23, 2016

@embray
Copy link
Contributor Author

embray commented Nov 23, 2016

@embray
Copy link
Contributor Author

embray commented Nov 23, 2016

@embray
Copy link
Contributor Author

embray commented Nov 23, 2016

@embray
Copy link
Contributor Author

embray commented Nov 23, 2016

Attachment: 0005-Ran.patch.gz

@embray
Copy link
Contributor Author

embray commented Nov 23, 2016

@embray
Copy link
Contributor Author

embray commented Nov 23, 2016

@mkoeppe
Copy link
Contributor

mkoeppe commented Nov 23, 2016

comment:1

Do you have a git repository from which you created this?

@kiwifb
Copy link
Member

kiwifb commented Nov 23, 2016

comment:2

I wouldn't have thought that ACLOCAL_AMFLAGS = -I m4 was necessary once you have AC_CONFIG_MACRO_DIR([m4]) but I could be wrong.

There are other things I am thinking off that can be done (VPATH) but we may not want to do everything at once.

@embray
Copy link
Contributor Author

embray commented Nov 24, 2016

comment:3

Replying to @kiwifb:

I wouldn't have thought that ACLOCAL_AMFLAGS = -I m4 was necessary once you have AC_CONFIG_MACRO_DIR([m4]) but I could be wrong.

I'm fairly sure myself that it isn't strictly necessary, but I put it because autoconf suggested that I do anyways.

@embray
Copy link
Contributor Author

embray commented Nov 24, 2016

comment:4

Or maybe it was libtoolize that told me that. I can't remember for sure. From https://www.gnu.org/software/libtool/manual/html_node/Invoking-libtoolize.html

"In the future other Autotools will automatically check the contents of AC_CONFIG_MACRO_DIRS, but at the moment it is more portable to add the macro directory to ACLOCAL_AMFLAGS in Makefile.am, which is where the tools currently look."

That future is now, but I guess it still suggests it just in case.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 2, 2017

comment:5

See also older tickets #15871, #15872.

@embray embray added the t: bug label Mar 3, 2017
@embray
Copy link
Contributor Author

embray commented Mar 3, 2017

comment:7

I think this is actually ready to go. I just don't know what the process is for adding a new tarball to the sage package mirrors. For this particular case I don't see a better workaround than a new source package since the original is a bit broken.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 3, 2017

comment:8

There's no branch, what is to be reviewed?

Also, could you just share the git from which these patches are generated? (comment 1)

Also, "cddlib-0.94.tar.gz" does not seem to have an unambiguous version number.

@embray
Copy link
Contributor Author

embray commented Mar 14, 2017

comment:10

What I don't understand is what the process is (though it seems like there is one?) of providing an alternative upstream tarball for one of the packages in Sage, such that is distributed to all the mirrors, etc.

I've attached such a tarball, as well as the patches that reproduce it from the original upstream tarball.

The problem here was that the source tarball itself was broken in various ways.

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 15, 2017

comment:11

Most of the process is described here: http://doc.sagemath.org/html/en/developer/packaging.html#when-to-patch-when-to-repackage-when-to-autoconfiscate (and in the following sections).

The release manager adds the "upstream tar ball" to the server when the ticket is merged. Usually people just provide an URL in the description, rather than attaching it to trac.

Instead of attaching patches here, use a public git repository. Make an spkg-src that can regenerate your "upstream tar ball" from the repository. See bliss/spkg-src for an example.

@embray
Copy link
Contributor Author

embray commented Apr 13, 2017

comment:12

Forgot about this until now, but thanks for the info. I'll follow the process you described then.

@saraedum
Copy link
Member

saraedum commented Apr 3, 2018

comment:13

embray: are you planning to work on this again anytime soon? I just stumbled upon this because Debian has some patches to fix Sage's integration of cddlib for version 0.94h.

If you think that the patches proposed here are still the way this should be done, should we propose them upstream?

@kiwifb
Copy link
Member

kiwifb commented Apr 3, 2018

comment:14

Just a note, from my experience in sage-on-gentoo, the debian patches are insufficient - as in I have doctest failures even with the debian patches.

Otherwise I am all for pushing updated autotooling upstream.

@embray
Copy link
Contributor Author

embray commented Apr 4, 2018

comment:15

This really needs #21196. IIRC I ran into some stumbling blocks with that, but I forget--it was a long time ago.

@saraedum
Copy link
Member

saraedum commented Apr 4, 2018

Dependencies: #21196

@saraedum
Copy link
Member

saraedum commented Apr 4, 2018

comment:16

So, that's a dependency if I understood you correctly.

@saraedum
Copy link
Member

comment:17

I'm dropping that dependency again, as I am in favor of dropping autotools from our SPKGs.

@saraedum
Copy link
Member

Changed dependencies from #21196 to none

@saraedum
Copy link
Member

comment:18

I wrote a mail to upstream, offering my help with this. Let's see what they say.

@saraedum
Copy link
Member

Changed upstream from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

@embray
Copy link
Contributor Author

embray commented Apr 23, 2018

comment:19

I think the series of patches I originally attached to this ticket should do it. I forget why I just added a bunch of patches here as opposed to some other way of doing it...

@saraedum
Copy link
Member

comment:20

Some of the patches don't apply anymore so I rebase them. I am trying to convince the author of the beauty of github (or similar) and he wants to have a look. I proposed a new home for upstream here: https://github.com/cddlib/cddlib.

And a modified version of your patches is here: cddlib/cddlib#1

Let's continue the discussion there.

@saraedum
Copy link
Member

Changed upstream from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

@saraedum saraedum modified the milestones: sage-7.5, sage-8.3 Apr 23, 2018
@saraedum
Copy link
Member

comment:23

Upstream has upgraded their autotooling. Now we just need to package the next release #25344.

@saraedum saraedum removed this from the sage-8.3 milestone May 11, 2018
@saraedum
Copy link
Member

comment:24

(feel free to reopen this, if you think that I am missing something here.)

@videlec
Copy link
Contributor

videlec commented May 18, 2018

comment:25

closing positively reviewed duplicates

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

5 participants