Skip to content
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

[R] Issues with configure script #35710

Closed
Enchufa2 opened this issue May 22, 2023 · 4 comments · Fixed by #36435
Closed

[R] Issues with configure script #35710

Enchufa2 opened this issue May 22, 2023 · 4 comments · Fixed by #36435

Comments

@Enchufa2
Copy link

Describe the bug, including details regarding any error messages, version, and platform.

I found two issues with the latest rewrite of the configure script for the R package:

  1. There's a new script to check whether the library version and the R package version are compatible. This is fine, but for arrow 11, there are versions 11.0.0.2 and 11.0.0.3 on CRAN. The script does not remove the fourth component, and thus it stops when it finds that 11.0.0 != 11.0.0.3. My understanding is that versions a.b.c.d of the R package should be considered compatible to a.b.c of the library.
  2. LIB_DIR is hardcoded here to something/lib. In many systems, this is lib64, not lib, and pkgconfig provides this info.

Component(s)

R

@Enchufa2 Enchufa2 changed the title Issues with configure script [R] Issues with configure script May 22, 2023
@thisisnic
Copy link
Member

@nealrichardson Would you mind taking a look at this one?

@nealrichardson
Copy link
Member

Thanks for the report!

  1. There's a new script to check whether the library version and the R package version are compatible. This is fine, but for arrow 11, there are versions 11.0.0.2 and 11.0.0.3 on CRAN. The script does not remove the fourth component, and thus it stops when it finds that 11.0.0 != 11.0.0.3. My understanding is that versions a.b.c.d of the R package should be considered compatible to a.b.c of the library.

Have you encountered a scenario where you have a C++ library with version x.0.0.2 and R package with x.0.0.3? The .d releases are typically things we have to do to patch the R package for CRAN, so there is no corresponding C++ release with .d.

  1. LIB_DIR is hardcoded here to something/lib. In many systems, this is lib64, not lib, and pkgconfig provides this info.

Good catch. It was always hard-coded for the non-pkg-config path, and I found that the previous way we were doing it with pkg-config, of using sed to get it from --libs-only-L, didn't behave well if there were other dirs in the search path, so I just dropped back to the hard-coded for all. But it looks like arrow.pc does include it as libdir, so we can get it as --variable=libdir for that case.

@Enchufa2
Copy link
Author

Have you encountered a scenario where you have a C++ library with version x.0.0.2 and R package with x.0.0.3?

No, I've encountered a scenario where I have a C++ library with version x.0.0 and R package with x.0.0.3. That happened with x=11.

The .d releases are typically things we have to do to patch the R package for CRAN, so there is no corresponding C++ release with .d.

Exactly, so my expectation is that R package x.0.0.3 should find C++ library version x.0.0 as compatible. And it does not currently.

@nealrichardson
Copy link
Member

Ah, got it, thanks

@kou kou closed this as completed in #36435 Jul 3, 2023
kou pushed a commit that referenced this issue Jul 3, 2023
### Rationale for this change

See #35710

### What changes are included in this PR?

* [Get `LIB_DIR` from `pkg-config` where possible](c8d09eb), to handle the possibility that it is `lib64` and not just `lib` on some platforms.
* [Allow x.y.z.1 to use x.y.z C++ library](a77f909), so that apt/yum official release packages can be used with patched versions submitted to CRAN.

### Are these changes tested?

The version check change has a unit test. The LIB_DIR change hopefully is well enough covered by our existing CI.

* Closes: #35710

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
@kou kou added this to the 13.0.0 milestone Jul 3, 2023
westonpace pushed a commit to westonpace/arrow that referenced this issue Jul 7, 2023
…ache#36435)

### Rationale for this change

See apache#35710

### What changes are included in this PR?

* [Get `LIB_DIR` from `pkg-config` where possible](apache@c8d09eb), to handle the possibility that it is `lib64` and not just `lib` on some platforms.
* [Allow x.y.z.1 to use x.y.z C++ library](apache@a77f909), so that apt/yum official release packages can be used with patched versions submitted to CRAN.

### Are these changes tested?

The version check change has a unit test. The LIB_DIR change hopefully is well enough covered by our existing CI.

* Closes: apache#35710

Authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants