feat: add musl target support #111

zitsen · 2018-09-04T15:20:19Z

Changes:

explicitly declare deps for bzip2 and lzma feature.
add musl target support using cc crate.

Though I choose to link native deps via the bzip2-sys and lzma-sys crates,
note that the deps could be done with cargo's target selector.
Let me know if you do not want to link it by default.

/cc #80

Changes: - explicitly declare deps for `bzip2` and `lzma` feature. - add musl target support using `cc` crate. Though I choose to link native deps with the `bzip2-sys` and `lzma-sys` crates, note that the deps could be done with cargo's `target` selector. Let me know if you do not want to link it by default. /cc rust-bio#80

Error description(see https://travis-ci.org/rust-bio/rust-htslib/builds/424648253): ``` kcov: warning: kcov: WARNING: kcov has been built without libbfd-dev (or kcov: binutils-dev), so the --verify option will not do anything. Can't set personality: Operation not permitted kcov: error: Can't start/attach to /home/travis/build/rust-bio/rust-htslib/target/debug/deps/rust_htslib-b4e39d181972c8d6 Child hasn't stopped: ff00 kcov: error: Can't start/attach to /home/travis/build/rust-bio/rust-htslib/target/debug/deps/rust_htslib-b4e39d181972c8d6 error: test failed ``` Reference: roblabla/cargo-travis#38

johanneskoester · 2018-09-05T08:42:37Z

Cargo.toml

@@ -13,6 +13,9 @@ documentation = "https://docs.rs/rust-htslib"

 [dependencies]
 libc = "0.2"
+libz-sys = "1.0"
+bzip2-sys = { version = "0.1.6", git = "https://github.com/alexcrichton/bzip2-rs.git", optional = true }


Not sure I understand this. I thought these sys crates are just the bindings, but we don't need bindings here do we?

Do they also ship the libs?

libz-sys is here

johanneskoester · 2018-09-05T08:44:11Z

build.rs

@@ -39,17 +48,28 @@ fn main() {
    if !use_bzip2 {
        let bzip2_patterns = vec!["s/ -lbz2//", "/#define HAVE_LIBBZ2/d"];
        sed_htslib_makefile(&out, &bzip2_patterns, "bzip2");
+    } else if let Ok(inc) = env::var("DEP_BZIP2_ROOT")


Why are these additional env vars needed?

Do they come from the bzip2-sys crate?

that's so sad I mistakenly deleted the commonts 😢 , maybe you could see them in email?

johanneskoester · 2018-09-05T08:50:10Z

Looks really nice! just a few questions of somebody not so experienced with C compilation.

zitsen · 2018-09-05T12:20:11Z

Explained here(:cry: seems that I'm not familiar with the github review behaviors...):

The -sys crate doest not only ship the libs, but also do lot of checks and settings, see the libz-sys build.rs, the cargo build script reference. At least these things to know:

detecting system library settings.
providing bidings of C API
set link options to rustc without forcely linked (I mean -lz ...) , support static link option- though only serveral could
providing include files and library files if system not match, eg. not exist or not static. That's the DEP_* environments come from.
cc is a cross platform compiler wraper, better to use it instead of use gcc command, it does more than a complie command line, more targets will be benifit from this.

johanneskoester

Amazing! Really nice and very good to know.

johanneskoester · 2018-09-05T12:25:05Z

Last question, does it make sense to follow a similar pattern for zlib as well (libz-sys)? If so, would you mind adding that here as well? If I understand correctly, this could make the build entirely self-contained.

zitsen · 2018-09-05T12:48:06Z

~~There's something to do following libz-sys style:~~

~~1. Windows support should be dealt with, test should be passed (via appveyor - it's upon to you).~~
~~2. More targets should be tested(this is optional)~~
~~3. Trust will help.~~
~~4. filling build.rs~~

~~I'm glad to help after your done with appveyor(it's free).~~

johanneskoester · 2018-09-06T08:02:38Z

Why would we need windows testing? As far as I know, htslib does not support windows, see here. Also, windows is not really relevant as a platform for the kind of bioinformatics analyses that can be done with htslib and rust-htslib.

zitsen · 2018-09-10T09:36:28Z

ignore it and make your decision then.

johanneskoester · 2018-09-10T15:02:10Z

Yes, I think libz-sys support would be nice. Do you want to add it in this PR?

zitsen · 2018-09-11T02:04:00Z

did you mean add libz-sys dep? it has already been included.

or libz-sys like build scripts for htslib???

maybe my english is urgly....

johanneskoester · 2018-09-12T08:05:11Z

I just meant adding at a dependency and using it. I was confused by the fact that there is no feature for it, and apparently it is not used in the build.rs. Is that intentional?

zitsen · 2018-09-12T08:49:44Z

see libz-sys added as a dependency here. in common, there's no need to declare it in build.rs, but do declare it in lib.rs, cargo link it automatically. bzip2 and lzma is declared as optional because of feature deps, but libz is always required.

any else?

johanneskoester · 2018-09-12T09:08:09Z

Thank you!

zitsen force-pushed the master branch 2 times, most recently from 03ff8cf to c999356 Compare September 5, 2018 01:02

zitsen force-pushed the master branch from c999356 to 89b9fbd Compare September 5, 2018 04:14

chore: not force install cargo-travis to save time

076586d

zitsen force-pushed the master branch 2 times, most recently from 9dae7ab to 640e70e Compare September 5, 2018 06:17

fix: try to fix default target error

b87736c

zitsen force-pushed the master branch from 640e70e to b87736c Compare September 5, 2018 06:21

johanneskoester reviewed Sep 5, 2018

View reviewed changes

johanneskoester approved these changes Sep 5, 2018

View reviewed changes

johanneskoester merged commit 1f9736e into rust-bio:master Sep 12, 2018

johanneskoester mentioned this pull request Sep 18, 2018

Unable to find libclang #112

Closed

wwood mentioned this pull request Apr 27, 2020

a question about building wwood/CoverM#29

Closed

This was referenced Mar 2, 2023

chore(master): release 0.41.0 #379

Closed

chore(master): release 0.41.1 #380

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add musl target support #111

feat: add musl target support #111

zitsen commented Sep 4, 2018

johanneskoester Sep 5, 2018

johanneskoester Sep 5, 2018

zitsen Sep 11, 2018

johanneskoester Sep 5, 2018

johanneskoester Sep 5, 2018

zitsen Sep 5, 2018

johanneskoester commented Sep 5, 2018

zitsen commented Sep 5, 2018 •

edited

Loading

johanneskoester left a comment

johanneskoester commented Sep 5, 2018 •

edited

Loading

zitsen commented Sep 5, 2018 •

edited

Loading

johanneskoester commented Sep 6, 2018

zitsen commented Sep 10, 2018

johanneskoester commented Sep 10, 2018

zitsen commented Sep 11, 2018

johanneskoester commented Sep 12, 2018

zitsen commented Sep 12, 2018 •

edited

Loading

johanneskoester commented Sep 12, 2018

feat: add musl target support #111

feat: add musl target support #111

Conversation

zitsen commented Sep 4, 2018

johanneskoester Sep 5, 2018

Choose a reason for hiding this comment

johanneskoester Sep 5, 2018

Choose a reason for hiding this comment

zitsen Sep 11, 2018

Choose a reason for hiding this comment

johanneskoester Sep 5, 2018

Choose a reason for hiding this comment

johanneskoester Sep 5, 2018

Choose a reason for hiding this comment

zitsen Sep 5, 2018

Choose a reason for hiding this comment

johanneskoester commented Sep 5, 2018

zitsen commented Sep 5, 2018 • edited Loading

johanneskoester left a comment

Choose a reason for hiding this comment

johanneskoester commented Sep 5, 2018 • edited Loading

zitsen commented Sep 5, 2018 • edited Loading

johanneskoester commented Sep 6, 2018

zitsen commented Sep 10, 2018

johanneskoester commented Sep 10, 2018

zitsen commented Sep 11, 2018

johanneskoester commented Sep 12, 2018

zitsen commented Sep 12, 2018 • edited Loading

johanneskoester commented Sep 12, 2018

zitsen commented Sep 5, 2018 •

edited

Loading

johanneskoester commented Sep 5, 2018 •

edited

Loading

zitsen commented Sep 5, 2018 •

edited

Loading

zitsen commented Sep 12, 2018 •

edited

Loading