-
Notifications
You must be signed in to change notification settings - Fork 83
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
Allow building in a local install #187
Conversation
@TomKellyGenetics Thanks for this! I'd be curious if your approach works for MUSL as well? Can you pull in #193, rebase and verify that your approach still works (with htslib 1.10.x series?). Cheers! |
@TomKellyGenetics Now you can pull |
Sorry I'm not sure why continuous integration checks are failing but it is merged from 'master' without much trouble for conflicts. It seems the version of |
Yes, you seem to hold an old version of that submodule, we are at 1.10.2 in |
Ok the issue seem to be using the submodule for version 1.9 https://github.com/rust-bio/htslib rather than the latest version 1.10.2 from https://github.com/samtools/htslib |
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.
wrapper.h
and wrapper.c
shouldn't need to change permission bits.
Sorry I misunderstood that the samtools/htslib submodule had already been updated. Pardon the messy commits, I've rebased the necessary changes. This should merge without changing the submodule. I'm not sure why clippy still gives errors. |
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.
CI seems to be failing still over here:
https://github.com/rust-bio/rust-htslib/pull/187/checks?check_run_id=740731648#step:6:204
Yes, I'm having trouble understanding why this happens here but not for PR #193. I've removed all non-essential changes so only the |
Checks seem to be passing now (conflicts with #193 resolved). I've made the changes on a fresh clone of |
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.
autoconf, configure, autoreconf and configure again? Wouldn't it be sensible to just leave the autoreconf && configure from master
?
Calling autoconf, configure, make, and make install were needed in the previous version. Compared to the new version of Note that this version forces a local install with a |
@brainstorm feel free to decide about this. Again, my experience with cross platform C is way too shallow to decide about such things. |
I'm actually a bit divided on this PR: on the one hand I would like My hope was that this PR would cater those users who still want to just install all deps manually, run @TomKellyGenetics If you can make sure that it builds well on all targets (OSX, Linux, Windows and MUSL (all of them dynamically and statically)), that'll be epic... hopefully PR #189 can fix most of that plus your help in here. OTOH Patrick mentions that the buildgen pre-building is not tested on windows so that'll need some testing and coverage on the GitHub Actions workflow. For all those intricacies, I'm a bit reluctant to merge this as-is right now since it can generate significant installation confusion among end users and developers :/ |
@brainstorm I understand, I'm also unsure if it will work on other platforms, especially as I haven't tested it on a windows system. Is it possible to do this on Appveyor? I'm not familiar enough with Rust to be sure this can be automated. It seems that As mentioned above, the initial motivation for this was to enable install on Mac (the root directory didn't work for some reason). Recent changes to I've also managed to build it |
@TomKellyGenetics The current GitHub actions setup does support windows, so in principle we shouldn't need to add AppVeyor, see: https://user-images.githubusercontent.com/175587/74093358-f6381d80-4b24-11ea-9bd7-1eec4fc7c7ee.png So with some proper testing and iteration we could have all platforms tested and building, but it does require some focused effort, I'm glad you got this one started 👍 |
@TomKellyGenetics I hope you understand, but I'm gonna close this for the reasons mentioned on #187 (comment) ... thanks for your input anyway, made me think about portability ;) |
No problem, I understand. I will leave the necessary changes here in case anyone wishes to do this as a workaround. This installs to a hardcoded directory in Unless there is a way to toggle toggle whether prefix is used (or pass as an argument) then I agree that it's better to keep diff --git a/hts-sys/Cargo.toml b/hts-sys/Cargo.toml
index 296a916..3e561d1 100644
--- a/hts-sys/Cargo.toml
+++ b/hts-sys/Cargo.toml
@@ -35,4 +35,5 @@ static = []
fs-utils = "1.1"
bindgen = { version = "0.53.2", default-features = false, features = ["runtime"] }
cc = "1.0"
-glob = "0.3.0"
\ No newline at end of file
+glob = "0.3.0"
+dirs = "1.0.2"
diff --git a/hts-sys/build.rs b/hts-sys/build.rs
index 66f745c..4b4df7a 100644
--- a/hts-sys/build.rs
+++ b/hts-sys/build.rs
@@ -6,6 +6,8 @@
use fs_utils::copy::copy_directory;
use glob::glob;
+use ::dirs::home_dir;
+
use std::env;
use std::fs;
use std::path::PathBuf;
@@ -86,8 +88,8 @@ fn main() {
.current_dir(out.join("htslib"))
.arg("clean")
.status().unwrap().success()
-
- &&
+
+ &&
!Command::new("autoreconf")
.current_dir(out.join("htslib"))
@@ -99,7 +101,8 @@ fn main() {
!Command::new("./configure")
.current_dir(out.join("htslib"))
.env("CFLAGS", &cc_cflags)
- .arg(format!("--host={}", &host))
+ .arg(format!("--host={}", &host))
+ .arg(format!("--prefix={}/local", home_dir().unwrap().into_os_string().into_string().unwrap()))
.status().unwrap().success()
{
panic!("could not configure htslib nor any of its plugins") |
Works on Mac OS and without sudo account.
Building the dependency
hts-sys/htslib
with Make does not work in Mac OS Mojave (10.14) or later. This enables installation of a local copy which essentially runs the configuration with one of these lines depending on the OS.Note: this has not been tested on Windows.