-
-
Notifications
You must be signed in to change notification settings - Fork 200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: do not rely on which
program during configuration
#1232
Conversation
Current Aviator status
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.
|
Second commits adds libxml2 library detection in the configure script. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on.
R says,
even though It is ironic that |
🤷 The upstream issue just mentions that they need "which" as a package dependency, not that we must fix here. |
e070566
to
4eb90d7
Compare
@krlmlr I think the current version should work. It should be reasonably safe to try to run these commands ( Note that for the library detection I requested the flags required when libxml2 is a static library. I believe this won't hurt when it is actually a shared library, but it will allow using a static one as well. If we only wanted shared ones, we'd need |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@@ -10,6 +10,6 @@ PKG_CPPFLAGS=-DUSING_R -I. -Ivendor -Ivendor/cigraph/src -Ivendor/cigraph/includ | |||
-DHAVE_GFORTRAN=1 \ | |||
-D_GNU_SOURCE=1 | |||
|
|||
PKG_LIBS = -lxml2 -lz -lglpk -lgmp $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS) | |||
PKG_LIBS = -lglpk -lgmp @libs@ $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order change may or may not be relevant. Let's see.
as it is not present on all systems
4eb90d7
to
ddfed18
Compare
@Antonov548 Do we actually need |
Don't really need it anymore #1235 |
Thanks for noticing the Nix PR! I think Nix is a great ecosystem for reproducible research and I am actively using it with R, but it does require updates as upstream packages change. Nix also has a very different way of approaching dependencies than most operating systems and package managers. https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/r-modules/default.nix contains most of the nix-specific tweaks we make to ensure R packages have the right system-level dependencies and settings. R package dependencies are automatically extracted from CRAN. I opened NixOS/nixpkgs#286406 because the error message was cryptic. If the build had immediately errored with "which - not found on system" the R ecosystem maintainers would have known exactly what to do. Once the next Of course, removing |
This PR avoids reliance on the
which
utility, which may not be present on all systems. It addresses complaints like this one: NixOS/nixpkgs#286406It also renamed
xml2_include_paths
tolilbxml2_cflags
to better reflect what this variable contains.