add gklib #12547

hadim · 2020-09-03T11:39:40Z

Checklist

Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml"
License file is packaged (see here for an example)
Source is from official source
Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged)
If static libraries are linked in, the license of the static library is packaged.
Build number is 0
A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details)
GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there

conda-forge-linter · 2020-09-03T11:39:45Z

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 (recipes/gklib) and found it was in an excellent condition.

hadim · 2020-09-03T12:16:54Z

@conda-forge/staged-recipes ready for review

recipes/gklib/meta.yaml

hadim · 2020-12-29T07:36:33Z

I don't know how to fix the windows build so I have disabled it for now. Besides that LGTM.

@conda-forge/help-c-cpp can you review please?

SimonBoothroyd · 2020-12-29T09:26:38Z

@hadim I think the build failures on windows are due to how shared libraries are generated on windows (some background here). We can probably patch the CMake files to make it work, but is there a reason you are building shared rather than static libraries (it should just work with static libraries)?

hadim · 2020-12-29T09:34:07Z

Trying to build with static now

hadim · 2020-12-29T09:49:12Z

Osx and linux are fine (only the tests fail due to static lib). I don't know what's wrong with win.

SimonBoothroyd · 2020-12-29T09:59:41Z

Hmm taking a look now.

SimonBoothroyd · 2020-12-29T10:30:47Z

@hadim the test failures seem to be just caused by syntax issues in the meta.yaml. I think changing them to something more like:

    - if not exist "%LIBRARY_INC%\\gk_arch.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_defs.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_externs.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_getopt.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\GKlib.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_macros.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_mkblas.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_mkmemory.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_mkpqueue2.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_mkpqueue.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_mkrandom.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_mksort.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_mkutils.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_proto.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gkregex.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_struct.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\gk_types.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\ms_inttypes.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\ms_stat.h" exit 1  # [win]
    - if not exist "%LIBRARY_INC%\\ms_stdint.h" exit 1  # [win]
    - if not exist "%LIBRARY_LIB%\\GKlib.lib" exit 1  # [win]
    - if not exist "%LIBRARY_BIN%\\cmpnbrs.exe" exit 1  # [win]
    - if not exist "%LIBRARY_BIN%\\csrcnv.exe" exit 1  # [win]
    - if not exist "%LIBRARY_BIN%\\fis.exe" exit 1  # [win]
    - if not exist "%LIBRARY_BIN%\\gkgraph.exe" exit 1  # [win]
    - if not exist "%LIBRARY_BIN%\\gkrw.exe" exit 1  # [win]
    - if not exist "%LIBRARY_BIN%\\m2mnbrs.exe" exit 1  # [win]

should fix them.

hadim · 2020-12-31T10:26:29Z

LGTM here.

SimonBoothroyd · 2020-12-31T11:06:44Z

Great - thanks for fixing those @hadim! Feel free to add me as a recipe maintainer also.

SimonBoothroyd · 2020-12-31T11:11:52Z

@conda-forge/staged-recipes, ready for review

hadim · 2021-01-02T09:52:08Z

@conda-forge/help-c-cpp please review

chrisburr · 2021-01-04T14:03:06Z

recipes/gklib/build.sh

+      -DCMAKE_PREFIX_PATH=$PREFIX \
+      -DDEBUG=OFF \
+      -DOPENMP=set \
+      -DBUILD_SHARED_LIBS=OFF \


Conda-forge has a strong preference for shared libraries to make it easier to manage updates and migrations for new ABIs (it also makes license compliance simplier).

Is it practical here? Or is there a reason to why static linking is really needed?

I think it would be practical on linux + OSX, but currently not on WIN as the __dllexport/__dllimport statements have not been properly set-up.

Should we package shared libraries for linux + OSX and static for WIN, or is it preferable to be consistent and keep everything static until everything can be built as shared?

Keep as much as shared as possible. You should get a working shared library on Windows by setting cmake -D CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS=ON.

@xhochy unfortunately the built binaries rely on a couple of global constants so CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS won't directly work here without patching.

Oh, amazing! CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS hasn't failed for me before.

Shared Unix builds are enabled in 1342d88.

recipes/gklib/meta.yaml

chrisburr · 2021-01-06T11:16:41Z

recipes/gklib/meta.yaml

+    - ${PREFIX}/bin/cmpnbrs --help  # [unix]
+    - ${PREFIX}/bin/csrcnv --help  # [unix]
+    - ${PREFIX}/bin/fis --help  # [unix]
+    - ${PREFIX}/bin/gkgraph --help  # [unix]
+    - ${PREFIX}/bin/gkrw --help  # [unix]
+    - ${PREFIX}/bin/m2mnbrs --help  # [unix]


They should be on PATH:

Suggested change

- ${PREFIX}/bin/cmpnbrs --help # [unix]

- ${PREFIX}/bin/csrcnv --help # [unix]

- ${PREFIX}/bin/fis --help # [unix]

- ${PREFIX}/bin/gkgraph --help # [unix]

- ${PREFIX}/bin/gkrw --help # [unix]

- ${PREFIX}/bin/m2mnbrs --help # [unix]

- cmpnbrs --help # [unix]

- csrcnv --help # [unix]

- fis --help # [unix]

- gkgraph --help # [unix]

- gkrw --help # [unix]

- m2mnbrs --help # [unix]

Oops thanks! Fixed by 38bf7b6

chrisburr · 2021-01-06T11:18:27Z

recipes/gklib/meta.yaml

@@ -11,7 +11,8 @@ source:

 build:
  number: 0
-  # skip: true  # [win]
+  run_exports:  # [not win]


You would also need to apply the selector to the pin_subpackage line as well though I'd suggest just removing them instead:

Suggested change

run_exports: # [not win]

run_exports:

(it will cause there to be a pointless dependency installed but it doesn't really matter and it has the benifit of tracking the ABI still)

Ah ok that makes sense - fixed by 38bf7b6.

chrisburr · 2021-01-06T11:19:02Z

recipes/gklib/meta.yaml

+    - if not exist "%LIBRARY_INC%\\ms_stat.h" exit 1  # [win]
+    - if not exist "%LIBRARY_INC%\\ms_stdint.h" exit 1  # [win]
+    - if not exist "%LIBRARY_LIB%\\GKlib.lib" exit 1  # [win]
+    - if not exist "%LIBRARY_BIN%\\cmpnbrs.exe" exit 1  # [win]


Can these be executed with --help as well?

Apparently they can! Fixed by 38bf7b6.

hadim commented Sep 3, 2020

View reviewed changes

recipes/gklib/meta.yaml Outdated Show resolved Hide resolved

This was referenced Sep 3, 2020

bump to 5.1.1 conda-forge/metis-feedstock#30

Closed

[WIP] add dgl #12552

Closed

hadim added 15 commits December 29, 2020 08:23

add gklib

a3ace54

WIP

9f40a01

WIP

74947c7

disable windows

281301c

use 5.1.1

92af008

try windows

4b8da7e

vc

1300b39

try with nmake

e9dbad0

WIP

33dc9d5

WIP

abce000

WIP

74daeb8

WIP

02ddfb5

disable windows again

be4cd34

tweak windows build

7d7da44

try windows again

991f0aa

hadim force-pushed the gklib branch from f4141d0 to 991f0aa Compare December 29, 2020 07:24

disable win

d32ae06

hadim requested a review from scopatz December 29, 2020 07:36

hadim mentioned this pull request Dec 29, 2020

Adds support for modern CMake KarypisLab/GKlib#8

Open

hadim added 3 commits December 29, 2020 10:32

Static

f70e59d

Update build.sh

991805f

Build win

1f6c0ad

SimonBoothroyd mentioned this pull request Dec 29, 2020

Add a git tag for the memory patches KarypisLab/METIS#24

Open

fix test

804a2a1

add SimonBoothroyd as maintainer

335bcc8

chrisburr reviewed Jan 4, 2021

View reviewed changes

SimonBoothroyd added 2 commits January 6, 2021 09:46

Build shared linux, add bin tests, add run_exports for unix.

1342d88

Fix unix library exists test.

dc5d3cd

chrisburr reviewed Jan 6, 2021

View reviewed changes

Remove run_exports selector, test windows commands.

38bf7b6

chrisburr merged commit aa8c463 into conda-forge:master Jan 7, 2021

hadim deleted the gklib branch January 7, 2021 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add gklib #12547

add gklib #12547

hadim commented Sep 3, 2020 •

edited

Loading

conda-forge-linter commented Sep 3, 2020

hadim commented Sep 3, 2020

hadim commented Dec 29, 2020

SimonBoothroyd commented Dec 29, 2020

hadim commented Dec 29, 2020

hadim commented Dec 29, 2020

SimonBoothroyd commented Dec 29, 2020

SimonBoothroyd commented Dec 29, 2020 •

edited

Loading

hadim commented Dec 31, 2020

SimonBoothroyd commented Dec 31, 2020

SimonBoothroyd commented Dec 31, 2020 •

edited

Loading

hadim commented Jan 2, 2021

chrisburr Jan 4, 2021

SimonBoothroyd Jan 5, 2021

xhochy Jan 5, 2021

SimonBoothroyd Jan 5, 2021

xhochy Jan 5, 2021

SimonBoothroyd Jan 6, 2021

chrisburr Jan 6, 2021

SimonBoothroyd Jan 6, 2021

chrisburr Jan 6, 2021

SimonBoothroyd Jan 6, 2021

chrisburr Jan 6, 2021

SimonBoothroyd Jan 6, 2021

add gklib #12547

add gklib #12547

Conversation

hadim commented Sep 3, 2020 • edited Loading

conda-forge-linter commented Sep 3, 2020

hadim commented Sep 3, 2020

hadim commented Dec 29, 2020

SimonBoothroyd commented Dec 29, 2020

hadim commented Dec 29, 2020

hadim commented Dec 29, 2020

SimonBoothroyd commented Dec 29, 2020

SimonBoothroyd commented Dec 29, 2020 • edited Loading

hadim commented Dec 31, 2020

SimonBoothroyd commented Dec 31, 2020

SimonBoothroyd commented Dec 31, 2020 • edited Loading

hadim commented Jan 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadim commented Sep 3, 2020 •

edited

Loading

SimonBoothroyd commented Dec 29, 2020 •

edited

Loading

SimonBoothroyd commented Dec 31, 2020 •

edited

Loading