Source files for vendored libraries by Antonov548 · Pull Request #1156 · igraph/rigraph

Antonov548 · 2024-01-23T13:51:14Z

Closes #1126.

aviator-app · 2024-01-23T13:51:17Z

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes.
Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This PR was merged using Aviator.

See the real-time status of this PR on the Aviator webapp.

Use the Aviator Chrome Extension to see the status of your PR within GitHub.

krlmlr · 2024-01-23T14:21:39Z

Thanks. I'll review, can we discuss at around 16:00 if needed?

Antonov548 · 2024-01-23T14:30:19Z

Thanks. I'll review, can we discuss at around 16:00 if needed?

Yes, sure. I trying to find a way to check that glpk in installed, but seems there is no straight forward solution.

szhorvat · 2024-01-23T17:50:29Z

configure

+  PKG_LIBS="$PKG_LIBS -lglpk"
+else
+  echo "Using vendored GLPK"
+  PKG_CFLAGS="$PKG_CFLAGS -Ivendor/cigraph/vendor/glpk -Ivendor/cigraph/vendor/glpk/env -Ivendor/cigraph/vendor/glpk/minisat -Ivendor/cigraph/vendor/glpk/misc -Ivendor/cigraph/vendor/glpk/draft -Ivendor/cigraph/vendor/glpk/npp -Ivendor/cigraph/vendor/glpk/api -Ivendor/cigraph/vendor/glpk/mpl -Ivendor/cigraph/vendor/glpk/bflib -Ivendor/cigraph/vendor/glpk/amd -Ivendor/cigraph/vendor/glpk/simplex -Ivendor/cigraph/vendor/glpk/colamd"


-I flags should go in CPPFLAGS instead of CFLAGS, no?

Need to define @cppflags below.

Is this comment just about naming? As I understand it doesn't has functional meaning.

We can leave it as is and resolve later if needed.

src/Makevars.in

krlmlr · 2024-02-08T20:08:08Z

The PR has conflicts now.

maelle · 2024-02-12T08:58:02Z

@Antonov548 is any help needed to fix the conflicts?

Antonov548 · 2024-02-12T09:07:39Z

@Antonov548 is any help needed to fix the conflicts?

I think it's better to start from scratch. Since part was already implemented in #1233

maelle · 2024-02-12T09:27:36Z

Ok so you want to close this PR and start a new one? I'm a bit lost as I don't know how you're generating the PR. (but I can learn!)

Antonov548 · 2024-02-12T09:33:10Z

Ok so you want to close this PR and start a new one? I'm a bit lost as I don't know how you're generating the PR. (but I can learn!)

Yes, I will create new one. Just need to clarify what still needs to be done regarding vendored libraries, since as I mentioned part is already done.

Antonov548 · 2024-02-13T10:48:23Z

@krlmlr Seems it couldn't detect glpk on pipeline just with commpilation single file. Not sure why it happens. But using vendored glpk also doesn't work properlly now. Not all files from vendor/cigraph/vendor/glpk should be used.

krlmlr · 2024-02-13T08:16:52Z

configure

+  PKG_LIBS="$PKG_LIBS -lglpk"
+else
+  echo "Using vendored GLPK"
+  PKG_CFLAGS="$PKG_CFLAGS -Ivendor/cigraph/vendor/glpk -Ivendor/cigraph/vendor/glpk/env -Ivendor/cigraph/vendor/glpk/minisat -Ivendor/cigraph/vendor/glpk/misc -Ivendor/cigraph/vendor/glpk/draft -Ivendor/cigraph/vendor/glpk/npp -Ivendor/cigraph/vendor/glpk/api -Ivendor/cigraph/vendor/glpk/mpl -Ivendor/cigraph/vendor/glpk/bflib -Ivendor/cigraph/vendor/glpk/amd -Ivendor/cigraph/vendor/glpk/simplex -Ivendor/cigraph/vendor/glpk/colamd"


Need to define @cppflags below.

krlmlr · 2024-02-13T08:17:07Z

rconfigure.py

+
+generate_sources('/', 'sources', 'SOURCES', ignore_folders)
+generate_sources('/vendor/cigraph/vendor/glpk', 'sources-glpk', 'GLPKSOURCES', [], ('.c', '.cc', '.cpp'))
+generate_sources('/vendor/cigraph/vendor/mini-gmp', 'sources-mini-gmp', 'MINIGMPSOURCES')


No longer needed.

Suggested change

generate_sources('/vendor/cigraph/vendor/mini-gmp', 'sources-mini-gmp', 'MINIGMPSOURCES')

Still needed here, but let's remove mini-gmp from ignore_folders in a separate PR.

src/Makevars.in

krlmlr · 2024-02-13T10:49:20Z

configure

+  PKG_LIBS="$PKG_LIBS -lglpk"
+else
+  echo "Using vendored GLPK"
+  PKG_CFLAGS="$PKG_CFLAGS -Ivendor/cigraph/vendor/glpk -Ivendor/cigraph/vendor/glpk/env -Ivendor/cigraph/vendor/glpk/minisat -Ivendor/cigraph/vendor/glpk/misc -Ivendor/cigraph/vendor/glpk/draft -Ivendor/cigraph/vendor/glpk/npp -Ivendor/cigraph/vendor/glpk/api -Ivendor/cigraph/vendor/glpk/mpl -Ivendor/cigraph/vendor/glpk/bflib -Ivendor/cigraph/vendor/glpk/amd -Ivendor/cigraph/vendor/glpk/simplex -Ivendor/cigraph/vendor/glpk/colamd"


We can leave it as is and resolve later if needed.

Antonov548 · 2024-02-13T10:59:26Z

Oh, I missed. It's has errors in checking code rather than build the package. Can we try to avoid this issues with not including files which raise the warnings?

Antonov548 · 2024-02-13T13:34:53Z

@krlmlr is there any way to ignore warnings in vendor folder for the check step?

P.S. still need to figure out why vendor version is chosen in GHA.

krlmlr · 2024-02-20T13:56:36Z

@Antonov548: Turning off warnings for certain parts of the code will be difficult.

@szhorvat: How did this even work with pre-2.0.0 without GLPK installed? Did R CMD check complain then?

Only CRAN is wary about these warnings, but they will never see them because they have GLPK installed. (Or so I hope.) We could just ignore those warnings on GHA, but we need multiple runs then: one with, and one without GLPK.

szhorvat · 2024-02-20T13:57:35Z

How did this even work with pre-2.0.0 without GLPK installed?

It disabled GLPK.

Do we need to care about R CMD check for anything except CRAN? CRAN has GLPK.

We do turn off some warnings for the vendored GLPK with the C core's build system. It does use some outdated coding practices. GLPK has no issue tracker, not even a public repository. It is being developed the old-fashioned way on a GNU mailing list. I wouldn't take this up with upstream.

szhorvat · 2024-02-20T14:11:43Z

I should point out that we go to great lengths to make sure that igraph doesn't print without permission. So the complaints about printf being referenced are theoretical, and only relevant for avoiding an argument with CRAN.

As for patching out all the printfs in the vendored GLPK: we don't do that because it makes it much more painful to upgrade GLPK when a new version comes out. Generally, we try to keep patches to vendored code to a minimum for this reason.

Antonov548 · 2024-02-21T20:34:12Z

@krlmlr I was wondering why on GHA it use vendored version of glpk even if it's installed.

I found that the problem is in using R command in configure. During the check using rcmdcheck R command is not available. They have it here: https://cran.r-project.org/doc/manuals/r-devel/R-exts.html#Configure-example
Do not invoke R by plain R, Rscript or...

I have changed it to use "${R_HOME}/bin/R" as it was suggested in the same webpage.
It also works with vendored glpk

DESCRIPTION

krlmlr · 2024-02-27T08:14:34Z

rconfigure.py

+
+generate_sources('/', 'sources', 'SOURCES', ignore_folders)
+generate_sources('/vendor/cigraph/vendor/glpk', 'sources-glpk', 'GLPKSOURCES', [], ('.c', '.cc', '.cpp'))
+generate_sources('/vendor/cigraph/vendor/mini-gmp', 'sources-mini-gmp', 'MINIGMPSOURCES')


Still needed here, but let's remove mini-gmp from ignore_folders in a separate PR.

Antonov548 requested a review from krlmlr January 23, 2024 13:51

Antonov548 changed the title ~~Sources file for vendored libraries~~ Source files for vendored libraries Jan 23, 2024

krlmlr force-pushed the feat/split-sources branch 2 times, most recently from 2aec3e7 to 0a2a6d6 Compare January 23, 2024 15:21

szhorvat reviewed Jan 23, 2024

View reviewed changes

krlmlr added this to the upgrade-2 milestone Jan 23, 2024

szhorvat reviewed Jan 23, 2024

View reviewed changes

src/Makevars.in Outdated Show resolved Hide resolved

maelle assigned Antonov548 Jan 29, 2024

Antonov548 marked this pull request as draft January 29, 2024 12:14

szhorvat reviewed Feb 3, 2024

View reviewed changes

src/Makevars.in Outdated Show resolved Hide resolved

krlmlr modified the milestones: upgrade-2, phoenix 🔥 Feb 3, 2024

szhorvat mentioned this pull request Feb 7, 2024

fix: configure manage libxml multiple include paths #1197

Merged

Antonov548 closed this Feb 12, 2024

krlmlr reopened this Feb 13, 2024

krlmlr force-pushed the feat/split-sources branch 6 times, most recently from b754a98 to 724985c Compare February 13, 2024 08:22

krlmlr reviewed Feb 13, 2024

View reviewed changes

krlmlr modified the milestones: phoenix 🔥, upkeep, upgrade-2 Feb 20, 2024

Antonov548 marked this pull request as ready for review February 22, 2024 19:02

krlmlr approved these changes Feb 27, 2024

View reviewed changes

krlmlr added the mergequeue label Feb 27, 2024

krlmlr and others added 11 commits February 27, 2024 08:16

Make glpk optional again

26107b3

Optional

62efff4

fix: add minigmp sources/libxml library

82ebb96

fix: windows include mini-gmp

a8c13a0

fix: mini-gmp also in ucrt

8056375

revert me: not optional glpk

445367c

revert me: tmate

920dbfa

revert

cb9f3c6

fix: CC compiler in configure

31dab3e

regenerate sources

8e05ed7

Apply suggestions from code review

ab22467

aviator-app bot force-pushed the feat/split-sources branch from 7624a3e to ab22467 Compare February 27, 2024 08:16

aviator-app bot merged commit 990a73b into main Feb 27, 2024

aviator-app bot deleted the feat/split-sources branch February 27, 2024 08:49

github-actions bot locked as resolved and limited conversation to collaborators Feb 27, 2025

Uh oh!

Conversation

Antonov548 commented Jan 23, 2024 • edited by krlmlr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aviator-app bot commented Jan 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Aviator status

Uh oh!

krlmlr commented Jan 23, 2024

Uh oh!

Antonov548 commented Jan 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

krlmlr commented Feb 8, 2024

Uh oh!

maelle commented Feb 12, 2024

Uh oh!

Antonov548 commented Feb 12, 2024

Uh oh!

maelle commented Feb 12, 2024

Uh oh!

Antonov548 commented Feb 12, 2024

Uh oh!

Antonov548 commented Feb 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Antonov548 commented Feb 13, 2024

Uh oh!

Antonov548 commented Feb 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krlmlr commented Feb 20, 2024

Uh oh!

szhorvat commented Feb 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szhorvat commented Feb 20, 2024

Uh oh!

Antonov548 commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Antonov548 commented Jan 23, 2024 •

edited by krlmlr

Loading

aviator-app bot commented Jan 23, 2024 •

edited

Loading

Antonov548 commented Feb 13, 2024 •

edited

Loading

szhorvat commented Feb 20, 2024 •

edited

Loading

Antonov548 commented Feb 21, 2024 •

edited

Loading