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

Allow building in a local install #187

Closed
wants to merge 3 commits into from

Conversation

TomKellyGenetics
Copy link
Member

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.

./configure --prefix=/Users/$USER/local
./configure --prefix=/home/$USER/local

Note: this has not been tested on Windows.

@brainstorm
Copy link
Member

@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!

@brainstorm
Copy link
Member

@TomKellyGenetics Now you can pull master instead of that PR and rebase, that PR has been merged :)

@TomKellyGenetics
Copy link
Member Author

TomKellyGenetics commented Jun 4, 2020

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 hts-sys/htslib submodule is 1.9. Is this outdated?

@brainstorm
Copy link
Member

brainstorm commented Jun 4, 2020

Yes, you seem to hold an old version of that submodule, we are at 1.10.2 in master, that's one of the reasons you see errors on CI (other than some other clippy issues).

@TomKellyGenetics
Copy link
Member Author

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
Seems like a few more warnings from clippy still.

Copy link
Member

@brainstorm brainstorm left a 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.

@TomKellyGenetics
Copy link
Member Author

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.

Copy link
Member

@brainstorm brainstorm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TomKellyGenetics
Copy link
Member Author

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 hts-sys/build.rs script is different but it still returns an error.

@TomKellyGenetics
Copy link
Member Author

Checks seem to be passing now (conflicts with #193 resolved). I've made the changes on a fresh clone of master branch to ensure no changes to submodules, etc.

Copy link
Member

@brainstorm brainstorm left a 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?

@TomKellyGenetics
Copy link
Member Author

Calling autoconf, configure, make, and make install were needed in the previous version. Compared to the new version of master which supports MUSL, I think this may not longer be necessary.

Note that this version forces a local install with a ./configure --prefix argument. It would be ideal to have this as a conditional.

@johanneskoester
Copy link
Contributor

@brainstorm feel free to decide about this. Again, my experience with cross platform C is way too shallow to decide about such things.

@brainstorm
Copy link
Member

I'm actually a bit divided on this PR: on the one hand I would like rust-htslib to be buildable locally in an easy way, but as we are learning on PR #189 and we learned on PRs #184 and #193, it is indeed pretty hard to build on all targets without docker... and even if we go for docker, we have the nasty OSX side effect of #202, where we cannot easily abstract the build process away by using docker :/

My hope was that this PR would cater those users who still want to just install all deps manually, run cargo build and get to work.

@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 :/

@TomKellyGenetics
Copy link
Member Author

TomKellyGenetics commented Jun 10, 2020

@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 dirs::home_dir() is supported on Windows as shown here but I haven't tested local build on Windows.

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 master may have addressed some of these issues. Without further testing, I'm okay with this PR remaining unmerged as a workaround for those that need to make similar changes for local install to work.

I've also managed to build it rust-htlslib within a docker container and in this case, these changes weren't necessary.

@brainstorm
Copy link
Member

brainstorm commented Jun 11, 2020

@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 👍

@brainstorm
Copy link
Member

@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 ;)

@brainstorm brainstorm closed this Aug 4, 2020
@TomKellyGenetics
Copy link
Member Author

TomKellyGenetics commented Aug 4, 2020

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 ~/local/.

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 master as is.

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")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants