Skip to content

Commit

Permalink
Update onig_sys Version & Remove Deprecated Feature Flag
Browse files Browse the repository at this point in the history 
Reference the latest version of `onig_sys`. Update the feature flags
to remove the `static-libonig` flag. This has been deprecated for a
while. This is still a breaking change, so will be a major version bump.
  • Loading branch information
@iwillspeak
iwillspeak committed Sep 16, 2017
1 parent 947357e commit 679348d
Show file tree
Hide file tree
Showing 4 changed files with 15 additions and 16 deletions.
10 changes: 2 additions & 8 deletions Cargo.toml
View file
Expand Up @@ -15,18 +15,12 @@ license = "MIT"

[features]
std-pattern = []
static-libonig = ["onig_sys/static_onig"]

This comment has been minimized.

Sign in to view

Copy link
@epage

epage Nov 13, 2017

Was there a reason this flag was removed? For clients, its much nicer to use feature flags than having to setup environment variables.

This comment has been minimized.

Sign in to view

Copy link
@iwillspeak

iwillspeak Nov 14, 2017

Author Collaborator

This was part of #38. The idea behind that was that feature flags shouldn't control things like this. The advice was given at the bottom of this comment.

This comment has been minimized.

Sign in to view

Copy link
@trishume

trishume Nov 16, 2017

Contributor

@iwillspeak Interesting. I don't understand why it's bad since @retep998 didn't explain, but I guess I'll trust them for now.

Instead I'll ask a different question, what are the disadvantages of making static linking the default? AFAIK what happens with dynamic linking is that onig_sys builds oniguruma in the cargo build directory and then the binary refers to the binary in that location, which means your binary depends on your source folder staying in the same place to work, which seems bad. I don't really know of any advantages for dynamic linking, you're saving a tiny amount of space on your hard disk, but making it impossible to distribute the binary or delete/move the cargo build directory.

I looked on crates.io and it seems that all the things that depend on onig (mostly transitive through syntect) are things that mostly are tools that would want to distribute static binaries.

To me static sounds like the right default, but it might just be that I don't know about some advantage of dynamic linking.

This comment has been minimized.

Sign in to view

Copy link
@retep998

retep998 Nov 16, 2017

The reason to use environment variables for this over feature flags is because you're not changing the API surface of the crate, you're merely changing how some native library is linked, which is something that upstream library dependencies do not care about. Why should syntect care whether onig_sys is statically linked or dynamically linked?

Only the top level binary crate actually cares about how native libraries are linked, and when your -sys crate is some transitive dependency several layers deep, the top level crate has a really hard time controlling the feature flags of your -sys crate. An environment variable, on the other hand, is easily controllable by the person compiling that top level binary crate.

This comment has been minimized.

Sign in to view

Copy link
@epage

epage Nov 16, 2017

My assumption on another reason is I've heard that some distributions are a bit obsessive in preferring dynamic linking over static linking, so this provides them an escape hatch to force included binaries to use dynamic linking.

Only the top level binary crate actually cares about how native libraries are linked, and when your -sys crate is some transitive dependency several layers deep, the top level crate has a really hard time controlling the feature flags of your -sys crate.

While I've not messed with this feature, isn't the target type an equivalent and isn't that set by the dependency rather than the top-level?

If we defer the decision to the top-level crate, then the top-level crate has to start worrying about what all its leaf dependencies are doing which is quite an undertaking in rust with how granular things are.

I can't even imagine how us top level crate maintainers are expected to track that. Does every crate that depends on a -sys crate, advertise the build flags of its dependencies? Do I look at my lock file to find every -sys crate, find their web pages, and try to see if they do a good job advertising their build flags?

To me static sounds like the right default, but it might just be that I don't know about some advantage of dynamic linking

Defaults are important. If we defer to the toplevel, we should consider what is the default of least surprise to try to minimize the maintenance burden mentioned above.

Static linking makes the -sys crate act the the most like any other crate, so that would be my preference.

This comment has been minimized.

Sign in to view

Copy link
@trishume

trishume Nov 16, 2017

Contributor

The thing is as far as I can tell the normal arguments about dynamic linking, especially in Linux distros, don't apply here since onig-sys builds its own copy of oniguruma and never uses a system one (I could be wrong here).

I buy the argument about the top level should have the final say, but also that the top level shouldn't need to know what the build flags of a transitive dependency are. I think the answer is that there is just no great way of doing options like this, and the real reason we're running into problems is that the default is wrong in this case and leads to a bunch of pain if you don't override it.

This comment has been minimized.

Sign in to view

Copy link
@iwillspeak

iwillspeak Nov 17, 2017

Author Collaborator

since onig-sys builds its own copy of oniguruma and never uses a system one

onig-sys will use the system version if it can find it with pkg-config.

Static linking makes the -sys crate act the the most like any other crate

That's interesting. I was trying to link to the library in whatever was the 'default' way for that platform (static on MUSL, dynamic on Linux etc.). However making onig-sys behave like any other crate is an interesting justification for defaulting to static linking for all builds. Not sure if that can be fully justified though as dynamic linking does have its benefits.

I've crated #65 to continue this discussion.

[dependencies]
libc = "0.2"
bitflags = "1.0"
lazy_static = "0.2"

[target.'cfg(not(target_env = "musl"))'.dependencies.onig_sys]
version = "65.0.1"
path = "onig_sys"

[target.'cfg(target_env = "musl")'.dependencies.onig_sys]
features = [ "static_onig"]
version = "65.0.1"
[dependencies.onig_sys]
version = "66.1.0"
path = "onig_sys"
3 changes: 0 additions & 3 deletions onig_sys/Cargo.toml
View file
Expand Up @@ -17,9 +17,6 @@ documentation = "http://rust-onig.github.io/rust-onig/onig_sys/"
readme = "../README.md"
license = "MIT"

[features]
static_onig = []

[dependencies]
libc = "0.2"

Expand Down
16 changes: 12 additions & 4 deletions onig_sys/build.rs
View file
Expand Up @@ -77,13 +77,21 @@ pub fn compile(static_link: bool) {
lib_name);
}

#[cfg(not(target_env = "musl"))]
fn should_static_link() -> bool {
env::var("CARGO_FEATURE_STATIC_ONIG").is_ok() ||
env::var("RUSTONIG_STATIC_LIBONIG").is_ok()
}

#[cfg(target_env = "musl")]
fn should_static_link() -> bool {
true
}

pub fn main() {
if let Ok(_) = pkg_config::find_library("oniguruma") {
return;
}

let static_link = env::var("CARGO_FEATURE_STATIC_ONIG").is_ok() ||
env::var("RUSTONIG_STATIC_LIBONIG").is_ok();

compile(static_link);
compile(should_static_link());
}
2 changes: 1 addition & 1 deletion src/utils.rs
View file
Expand Up @@ -51,7 +51,7 @@ mod tests {
#[test]
pub fn utils_get_version_returns_expected_version() {
let version = version();
assert_eq!(version, "6.5.0");
assert_eq!(version, "6.6.1");
}

#[test]
Expand Down

0 comments on commit 679348d

Please sign in to comment.