Skip to content

refactor: always use bundled mini-gmp#1233

Merged
aviator-app[bot] merged 2 commits intomainfrom
experiment/mini-gmp
Feb 11, 2024
Merged

refactor: always use bundled mini-gmp#1233
aviator-app[bot] merged 2 commits intomainfrom
experiment/mini-gmp

Conversation

@szhorvat
Copy link
Copy Markdown
Member

@szhorvat szhorvat commented Feb 11, 2024

This is an experiment to see if using the bundled mini-gmp triggers any warnings CRAN would complain about. I noticed that we are already compiling mini-gmp.c even though we were not using it.

There are currently no disadvantages to using mini-gmp instead of the full-blown GMP, other than the risk of CRAN demanding source changes to fix warnings. We should not change mini-gmp ourselves it's too risky.

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Feb 11, 2024

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.

@szhorvat
Copy link
Copy Markdown
Member Author

This seems to trigger no warnings. Hopefully the LTO run wouldn't either.

Since we were already compiling mini-gmp.c, it is likely that we were inadvertently using it instead of an external GMP anyway.

Therefore I suggest merging this and using mini-gmp only until igraph gains features for which an external GMP has tangible benefits.

@szhorvat szhorvat marked this pull request as ready for review February 11, 2024 17:56
@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Feb 11, 2024

If this is merged, that's 1 out of 3 done. libxml2 should be easy too. That would leave GLPK as the only difficult one to detect.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 11, 2024

Thanks. We also need to edit DESCRIPTION to indicate that GMP is no longer needed.

Checking LTO now.

@szhorvat
Copy link
Copy Markdown
Member Author

@krlmlr Do you think it would be better to move defining INTERNAL_GMP to src/vendor/config.h, where it normally lives with C/igraph? Right now it's confusing that that file has /* #undef INTERNAL_GMP */. I think I'll just move it there together with fixing DESCRIPTION

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 11, 2024

No preference, but consistency seems important. What about the other includes that also might be misplaced?

@szhorvat szhorvat force-pushed the experiment/mini-gmp branch from e34113b to 2606369 Compare February 11, 2024 18:07
@szhorvat
Copy link
Copy Markdown
Member Author

Will fix those in a separate commit, within this PR. Give me 5 min.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 11, 2024

Why not do this in a completely separate PR instead?

@szhorvat
Copy link
Copy Markdown
Member Author

szhorvat commented Feb 11, 2024

Because it affects the same file, so the next PR would either depend on this, or conflict with this. Do you want me to remove that commit temporarily?

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 11, 2024

Let's continue here, it's fine. If the commits are clean and self-contained, that's good enough.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 11, 2024

LTO seems fine her too.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Feb 11, 2024

Would you like to finish the move to config.h here and add the "mergequeue" label?

@szhorvat
Copy link
Copy Markdown
Member Author

The move is finished in the sense that we're now consistent with the C core in what goes into config.h. Adding the label.

@aviator-app aviator-app bot force-pushed the experiment/mini-gmp branch from 87a577d to 04c7a57 Compare February 11, 2024 19:57
@aviator-app aviator-app bot merged commit 367c5ea into main Feb 11, 2024
@aviator-app aviator-app bot deleted the experiment/mini-gmp branch February 11, 2024 20:26
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 11, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants