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

Meta-ticket: Upgrade cddlib, fix cddlib header search for packages latte_int, gfan, topcom #29413

Closed
mkoeppe opened this issue Mar 27, 2020 · 47 comments

Comments

@mkoeppe
Copy link
Contributor

mkoeppe commented Mar 27, 2020

Follow up from #28905.

We upgrade to 0.94m: https://repology.org/project/cddlib/versions

Depends on #31482

CC: @orlitzky @dimpase @saraedum @slel @antonio-rojas @kiwifb @kliem

Component: packages: optional

Keywords: upgrade, cddlib

Author: Dima Pasechnik

Branch/Commit: 0c27c4f

Reviewer: Matthias Koeppe

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

@mkoeppe mkoeppe added this to the sage-9.1 milestone Mar 27, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.1, sage-9.2 Apr 18, 2020
@mkoeppe mkoeppe modified the milestones: sage-9.2, sage-9.3 Aug 29, 2020
@slel
Copy link
Member

slel commented Sep 15, 2020

Changed keywords from none to upgrade, cddlib

@slel
Copy link
Member

slel commented Sep 15, 2020

comment:3

New: cddlib 0.94k was released on 2020-09-15.

@dimpase
Copy link
Member

dimpase commented Dec 9, 2020

comment:4

a newer cdd 0.94m fixing #30319 has been released

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 25, 2021

comment:5

Are there already patches around for the cdd header file locations for gfan and topcom?

@mkoeppe

This comment has been minimized.

@kiwifb
Copy link
Member

kiwifb commented Feb 25, 2021

comment:6

For cddlib, this is a bit upsetting. We compile in Gentoo and in sage with -DNOCDDPREFIX which trigger block like this

#ifdef NOCDDPREFIX
#include "setoper.h"
#include "cdd.h"
#include "cdd_f.h"
#else
#include "cdd/setoper.h"
#include "cdd/cdd.h"
#include "cdd/cdd_f.h"
#endif

in src/lp_cdd.cpp, src/gfanlib_zcone.cpp and src/app_librarytest.cpp. So, it feels a bit stupid now that the prefix is cddlib. But at least the patch should be easy and then remove the -DNOCDDPREFIX from CXXFLAGS.

@kiwifb
Copy link
Member

kiwifb commented Feb 26, 2021

comment:7

nix has a patch to go with the removal of -DNOCDDPREFIX. However, there is a simpler fix. gfan only install binaries and no headers. Adding -Ipath_to/include/cddlib to CXXFLAGS will allow compilation and it is compatible with older cddlib too. I'll look at topcom next.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Feb 26, 2021

comment:8

Replying to @kiwifb:

However, there is a simpler fix. gfan only install binaries and no headers. Adding -Ipath_to/include/cddlib to CXXFLAGS will allow compilation and it is compatible with older cddlib too.

This is not a good fix. All of these packages should be modified (with proper upstreamable patches) so that they look for upstream's official new header name cddlib/cdd.h in the normal user-provided include path.

@kiwifb
Copy link
Member

kiwifb commented Feb 26, 2021

comment:9

That's easy enough to do. I would hope the fix has already been forwarded to the gfan people. Upstream is overdue for a maintenance release :)

@kiwifb
Copy link
Member

kiwifb commented Feb 26, 2021

comment:10

For topcom I am going to advocate adding to CPPFLAGS for now. Because upstream is a serial vendorer and its build system relies exclusively on vendored versions. Once we can get that upstream to get out of vendoring we may be able to suggest patches to them. But right now the point is moot.

@kiwifb
Copy link
Member

kiwifb commented Feb 26, 2021

comment:11

And topcom has a new version: 0.17.8 (still massively vendoring).

@kiwifb
Copy link
Member

kiwifb commented Feb 26, 2021

comment:12

Now that I see how much you already patched topcom, it is trivial to add a little love for cddlib at https://github.com/sagemath/sage-prod/blob/develop/build/pkgs/topcom/patches/configure.ac#L63

@kliem
Copy link
Contributor

kliem commented Feb 26, 2021

comment:13

I think I got it figured out for latte now: latte-int/latte#23

There is a couple of scenarios that could happen:

  • System cdd in:
    • inclusion directory
    • cdd/
    • cddlib'
  • Sages cdd in:
    • inclusion directory (of sage)
    • cddlib/ (once cdd is upgraded)

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 18, 2021

Dependencies: #31482

@mkoeppe

This comment has been minimized.

@mkoeppe mkoeppe changed the title Upgrade cddlib, fix cddlib header search for packages latte_int, gfan, topcom Meta-ticket: Upgrade cddlib, fix cddlib header search for packages latte_int, gfan, topcom Mar 18, 2021
@mkoeppe
Copy link
Contributor Author

mkoeppe commented Mar 18, 2021

comment:18

Also need to check polymake.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@mkoeppe

This comment has been minimized.

@jhpalmieri
Copy link
Member

comment:30

Is the last issue ("make singular pick sage's cdd") resolved? The Singular change is marked as "merged", and I've been using homebrew's cddlib and singular on at least one OS X machine for a while with no apparent issues.

@dimpase
Copy link
Member

dimpase commented Oct 3, 2022

comment:31

let's reenable cddlib for Homebrew.

@dimpase
Copy link
Member

dimpase commented Oct 3, 2022

Author: Dima Pasechnik

@dimpase
Copy link
Member

dimpase commented Oct 3, 2022

New commits:

add35c4allow Homebrew's cddlib

@dimpase
Copy link
Member

dimpase commented Oct 3, 2022

Branch: u/dimpase/configure/cdd_on_homebrew

@dimpase
Copy link
Member

dimpase commented Oct 3, 2022

Commit: add35c4

@jhpalmieri
Copy link
Member

comment:33

Good idea. Is there any reason to keep this meta-ticket open and handle homebrew/cddlib elsewhere? Otherwise, I'm happy with this change.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 5, 2022

comment:34

I think also the comment in build/pkgs/cddlib/spkg-configure.m4 should be updated so that it does not talk about this ticket as something for the future

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2022

Changed commit from add35c4 to 9ee86ae

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2022

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

9ee86aereflect the progress on #29413 in comments

@dimpase
Copy link
Member

dimpase commented Oct 5, 2022

comment:36

I've opened #34634 to deal with the renaming cddlib/->cdd/ of headers' subdir on Debian.

@mkoeppe

This comment has been minimized.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 5, 2022

comment:38

Another update on debian please for the comment

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2022

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

0c27c4fall good on Debian

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Oct 5, 2022

Changed commit from 9ee86ae to 0c27c4f

@dimpase
Copy link
Member

dimpase commented Oct 6, 2022

comment:40

Replying to Matthias Köppe:

Another update on debian please for the comment

done in ​0c27c4f

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 6, 2022

comment:41

Thanks. Should #31531 be a dependency?

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 6, 2022

Reviewer: Matthias Koeppe

@dimpase
Copy link
Member

dimpase commented Oct 6, 2022

comment:42

I don't see how #31531 is a dependency to this Homebrew-specific branch.

@mkoeppe
Copy link
Contributor Author

mkoeppe commented Oct 6, 2022

comment:43

ok then

@vbraun
Copy link
Member

vbraun commented Oct 16, 2022

Changed branch from u/dimpase/configure/cdd_on_homebrew to 0c27c4f

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

8 participants