Skip to content

Fixes for Makevars.in for unix (revert accidental file updates) #1092

Merged
aviator-app[bot] merged 1 commit intoigraph:mainfrom
jeroen:main
Jan 11, 2024
Merged

Fixes for Makevars.in for unix (revert accidental file updates) #1092
aviator-app[bot] merged 1 commit intoigraph:mainfrom
jeroen:main

Conversation

@jeroen
Copy link
Copy Markdown
Contributor

@jeroen jeroen commented Jan 10, 2024

I think you may have accidentally copied a wrong version of src/Makevars.in in #840. What you have now is a broken version of a Windows Makevars, not the unix one.

This PR restores it to the correct version from before #840 plus a small improvement to use the new sources.mk.

@aviator-app
Copy link
Copy Markdown
Contributor

aviator-app bot commented Jan 10, 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.

@jeroen jeroen changed the title Fix linking to lapack Fix Makevars.in for unix Jan 10, 2024
@jeroen jeroen changed the title Fix Makevars.in for unix Fix Makevars.in for unix (revert accidental file updates) Jan 10, 2024
@krlmlr krlmlr requested a review from Antonov548 January 10, 2024 16:46
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Jan 10, 2024

We're moving away from autoconf.

What exactly would you like to see changed? The mainline works well enough for us on GHA.

@jeroen
Copy link
Copy Markdown
Contributor Author

jeroen commented Jan 10, 2024

Your unix Makevars.in hardcodes variables from an old version of winbuilder. None of those variables actually exist, this unlikely to work on most platforms.

Also you are hardcoding -lstdc++ which breaks linux clang builds, which don't use libstdc++.

Also CMD check warns you that the way you link to lapack is wrong:

Warning:   apparently using $(LAPACK_LIBS) without following $(BLAS_LIBS) in ‘src/Makevars’
  apparently using $(BLAS_LIBS) without following $(FLIBS) in ‘src/Makevars’
  apparently using $(LAPACK_LIBS) without following $(BLAS_LIBS) in ‘src/Makevars.in’
  apparently using $(BLAS_LIBS) without following $(FLIBS) in ‘src/Makevars.in’

You should be using $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS) in PKG_LIBS as you did before.

Either way if this is intentional I'll leave it up to you to fix it, thought maybe it was a mistake.

@jeroen jeroen closed this Jan 10, 2024
@szhorvat
Copy link
Copy Markdown
Member

szhorvat commented Jan 10, 2024

It's hard to make do with plain makefiles without using all the auto-detection magic of either autoconf or CMake. Kirill, I know why you prefer simplicity (we all do when feasible), but the same level of portability that igraph's C core has simply isn't achievable without an advanced build system generator such as CMake or autoconf. If we don't do that in the long term, we will only be able to support simple and predictable environments, e.g. target CRAN only.

As for libstdc++, the only reliable way is to not link it explicitly, but find a way to use the C++ compiler to perform the linking (e.g. link with g++ instead of gcc). This is what CMake does as well. The C++ compiler knows what extra libraries it needs to link.

@jeroen
Copy link
Copy Markdown
Contributor Author

jeroen commented Jan 10, 2024

Instead of autoconf you can manually write a simple configure script that callspkg-config to populate Makevars.in like many other packages do, something like this: https://github.com/r-lib/ragg/blob/main/configure

As for libstdc++, the only reliable way is to not link it explicitly, but find a way to use the C++ compiler to perform

R will use the C++ linker by default because you have C++ files in your src dir. Just leave it out.

@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Jan 10, 2024

Thank you for your input. I'll leave this open for tracking.

@krlmlr krlmlr reopened this Jan 10, 2024
@krlmlr krlmlr added this to the upgrade milestone Jan 10, 2024
@jeroen
Copy link
Copy Markdown
Contributor Author

jeroen commented Jan 10, 2024

OK I have updated this PR to fix at least the urgent issues and fix the CMD check warning.

@jeroen jeroen changed the title Fix Makevars.in for unix (revert accidental file updates) Fixes for Makevars.in for unix (revert accidental file updates) Jan 10, 2024
@krlmlr
Copy link
Copy Markdown
Contributor

krlmlr commented Jan 11, 2024

Thanks, R CMD check looks much better now.

@krlmlr krlmlr mentioned this pull request Jan 11, 2024
@aviator-app aviator-app bot merged commit 7ce6d2f into igraph:main Jan 11, 2024
@jeroen
Copy link
Copy Markdown
Contributor Author

jeroen commented Jan 11, 2024

With this merged, at least the cross binaries are working again. So you can install macos-arm64 binaries from https://igraph.r-universe.dev/igraph

@szhorvat
Copy link
Copy Markdown
Member

szhorvat commented Feb 11, 2024

Instead of autoconf you can manually write a simple configure script that callspkg-config to populate Makevars.in like many other packages do

Many packages don't provide pkg-config files. GLPK does not and GMP only does with certain versions on certain systems. If GMP is actually MPIR (a compatible fork) then that's yet another story.

How would you detect these then in a way that is as robust as autoconf?

There's a reason why almost no C library that has non-trivial dependencies (like igraph) uses just a plain makefile. They usually use some build system generator like autoconf, CMake or meson.

@jeroen
Copy link
Copy Markdown
Contributor Author

jeroen commented Feb 11, 2024

How would you detect these then in a way that is as robust as autoconf?

autoconf is not magic, it just tries pkg-config and otherwise falls back to some default. You can accomplish the same without the bloat and complexity of autotools.

if pkg-config --exists gmp; then
GMP_LIBS=`pkg-config --libs gmp`
else 
GMP_LIBS="-lgmp"
fi

@szhorvat
Copy link
Copy Markdown
Member

szhorvat commented Feb 11, 2024

This does not detect if GMP is available, it simply assumes it. We need a way to actually detect it, and fall back to a vendored version otherwise. Note that the vendored version is not CRAN-friendly, so it can't be the default choice.

Many of our users are non-technical people. They are students, researchers, academics, in fields like sociology or biology. It is important that they be able to install igraph on Linux with as little effort as possible. Furthermore, many of them are stuck with using HPC clusters that don't have the prerequisite dependencies or use outdated OSs (CentOS 7 is common ...), and are managed by less-than-helpful sysadmins ...

@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