Skip to content

New packages ppl, pplpy (Parma polyhedra library), cysignals#4407

Merged
hoodmane merged 14 commits intopyodide:mainfrom
passagemath:ppl
Jan 27, 2024
Merged

New packages ppl, pplpy (Parma polyhedra library), cysignals#4407
hoodmane merged 14 commits intopyodide:mainfrom
passagemath:ppl

Conversation

@mkoeppe
Copy link
Copy Markdown
Contributor

@mkoeppe mkoeppe commented Jan 22, 2024

Description

Checklists

Copy link
Copy Markdown
Member

@ryanking13 ryanking13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mkoeppe! It looks like the test is failing because pplpy test is failing because it depends on gmpy2. We would also need some basic tests for pplpy.


# from https://pyodide.org/en/stable/development/building-from-sources.html#using-make:
# - build-essential
# we install file because it is used by packages/ppl during configure
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm a little hesitant about installing a tool to install a less common package, but file is a very common tool (I am quite surprised that it is not installed by default in the vscode devcontainer), so it's probably fine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, probably you can put it into environment.yml.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this particular case, the configure script appears to look for it specifically in /usr/bin, which is weird and could be patched out of course, but just installing in with apt is the easiest solution. As you say, file is a common tool; so it will certainly be present on typical developers' machines.

host:
- libgmp
- ppl
- gmpy2
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a python package need in runtime. So you need to put this in

requirements:
  run:
    - gmpy2

@mkoeppe mkoeppe changed the title New packages ppl, pplpy (Parma polyhedra library) New packages ppl, pplpy (Parma polyhedra library), cysignals Jan 25, 2024
@mkoeppe mkoeppe marked this pull request as draft January 25, 2024 04:58
@mkoeppe mkoeppe marked this pull request as ready for review January 26, 2024 19:02
@mkoeppe
Copy link
Copy Markdown
Contributor Author

mkoeppe commented Jan 26, 2024

Passes tests now, ready for review

Copy link
Copy Markdown
Member

@hoodmane hoodmane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mkoeppe! Generally looks good to me but i I'd appreciate it if you would add a bit more information about the patches.

Also it needs a changelog entry.

Comment on lines +1 to +4
From: Roberto Bagnara <bagnara@cs.unipr.it>
Date: Sun, 11 Feb 2018 08:11:09 +0000 (+0100)
Subject: Added missing "template" and "typename" keywords.
X-Git-Url: http://www.cs.unipr.it/git/gitweb.cgi?p=ppl%2Fppl.git;a=commitdiff_plain;h=c39f6a07b51f89e365b05ba4147aa2aa448febd7;hp=3a5e1e20a94cffb830182f22132b153d6691b7c5
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a backport? It's an old commit, surely there is a release since then. Are we pinning an old version for some reason? Are we expecting to get rid of it at some point?

Not a merge blocker but if you have answers to any of these questions it would be helpful to add them to the patch.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This version of PPL, 1.2, is unfortunately the latest upstream version - https://repology.org/project/ppl/versions
For the packages that I'm adding, I am using the patches carried by the SageMath distribution - see https://github.com/sagemath/sage/tree/develop/build/pkgs/ppl

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, could you copy this comment into the patch file below the original commit message?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ea991fd

Comment on lines +19 to +20
-#if HAVE_SIGALTSTACK
+#if HAVE_SIGALTSTACK && !defined(__EMSCRIPTEN__)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some more information here? What goes wrong without this patch? Compiler error? Runtime error? Should HAVE_SIGALTSTACK be false in Emscripten?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As indicated in the commit message with the patch, this is sagemath/cysignals#197, where I explain the details

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants