Some feedback on the meta.yml file

[fingolfin]

I have no idea how Conda or these "feedstocks" work, but some things here seem "odd" to me, and I thought this might be the easiest way to make the maintainers of this aware of it (background: I am a core GAP dev, and wrote the GAP build system).

[conda-forge-linter]

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

[fingolfin]

Another thing: I noticed that in your build.sh, you set custom CFLAGS but not custom CXXFLAGS, yet you then pass CXXFLAGS=$CXXFLAGS to configure. This won't matter in GAP 4.10, which doesn't use C++, but for GAP 4.11, it will.

[saraedum]

Another thing: I noticed that in your build.sh, you set custom CFLAGS but not custom CXXFLAGS, yet you then pass CXXFLAGS=$CXXFLAGS to configure. This won't matter in GAP 4.10, which doesn't use C++, but for GAP 4.11, it will.

The CXXFLAGS environment variable is set by our compiler package. There's probably nothing wrong there.

[saraedum]

@conda-forge-admin, please rerender.

[saraedum]

@fingolfin thanks for the feedback. I added your comments to our meta.yaml so we see them when we do the 4.11 upgrade.

[saraedum]

Btw, I suppose that much of the dependencies were taken from SageMath. There, they did probably build from an unreleased version at some point.

[fingolfin]

Uhm, I just see that you heavily edited this PR. My commit was labelled "WIP Do not merge". I'd rather not have that merged. If you want these changes, feel free to squash them, but please don't merge a commit with my name on it that was never meant to be merged.

[fingolfin]

Re CXXFLAGS: Well, you set a custom CFLAGS, but not a custom CXXFLAGS. As I wrote, this won't matter now. But it will matter in GAP 4.11, where you'd suddenly compile parts of the GAP codebase with different compiler flags. That does sound like a bug to me (albeit only a future one). Unless of course the default CFLAGS and CXXFLAGS match, and you custom CFLAGS are identical to the default CFLAGS -- but then why set custom CFLAGS at all?

[saraedum]

I can squash merge. Your name will show up in there though I guess. Not sure how github works exactly.

[saraedum]

@fingolfin a squash merge is ok with you then?

[fingolfin]

Another remark: a basic test of whether installation worked can be achieved via ./gap tst/testinstall.g, which sets the exit code to 0 or non-zero to indicate success/failure (in GAP 4.11, this can also be achieved by running make check). The test should require 1-2 minutes. Of course GAP needs to be startable, so the core packages (GAPDoc, smallgrp, primgrp, transgrp) must be installed.

[saraedum]

What files are needed for that test? Just that testinstall.g file or the whole test directory?

[saraedum]

Can we run this with the installed gap in the prefix or only with ./gap in the source tree?

[fingolfin]

The test suite is in tst, so you need all of it (well, at least tst/testinstall/ ...). An installed GAP can run it just fine, not just ./gap in the source tree

[saraedum]

@fingolfin a squash merge is ok with you then?

Ok. Let's see how the tests go. Squash is ok then even if your name might show up?

[fingolfin]

That's OK

[saraedum]

@fingolfin one error:

# Input is:
ShowPackageVariables("mockpkg");
# Expected output:
new global functions:
  mockpkg_GlobalFunction(  )*

new operations:
  mockpkg_Operation( arg )*

new attributes:
  mockpkg_Attribute( ... )*

new properties:
  mockpkg_Property( ... )*

new methods:
  mockpkg_Attribute( G )*
  mockpkg_Operation( G, n )*
  mockpkg_Property( ... )*

# But found:
other new globals (write protected):
  mockpkg_Attribute( ... )*
  mockpkg_GlobalFunction(  )*
  mockpkg_Operation( arg )*
  mockpkg_Property( ... )*

Is that something to worry about?

[saraedum]

@isuruf: Is this Python 3 compatible actually?

  File "share/gap/pkg/digraphs-0.15.2/scripts/travis-coverage.py", line 54
    print _WARN_PREFIX + 'Could not find .gi file to which this .tst refers\033[0m'
                     ^
SyntaxError: Missing parentheses in call to 'print'. Did you mean print(_WARN_PREFIX + 'Could not find .gi file to which this .tst refers\033[0m')?

  File "share/gap/pkg/jupyterviz-1.5.1/examples/convert.py", line 88
    print "Input file must have extension .md: " + mdfile
                                               ^
SyntaxError: Missing parentheses in call to 'print'. Did you mean print("Input file must have extension .md: " + mdfile)?

  File "share/gap/pkg/jupyterviz-1.5.1/extract_examples.py", line 73
    print "Processing " + gdfile + "..."
                      ^
SyntaxError: Missing parentheses in call to 'print'. Did you mean print("Processing " + gdfile + "...")?