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

make sure build.rs is re-run, when env vars change #694

Merged
merged 6 commits into from
Oct 21, 2022

Conversation

drahnr
Copy link
Contributor

@drahnr drahnr commented Oct 11, 2022

Currently a change in linkage mode env vars will not trigger a re-run of the build.rs-binary, and hence compilations fail until the that binary is deleted or other triggers are manually triggered.

To my understanding, it's a mere lag of 3 lines telling cargo to re-run the build script on env var change.

Also adds the option to NOT link statically if i.e. ROCKSDB_STATIC=false is provided

Some(_) => "static",
None => "dylib",
let mode = match env::var(&format!("{}_STATIC", lib_name)).as_ref().ok().map(|s| s.as_str()) {
Some("") | Some("true") | Some("1") | Some("static") => "static",
Copy link
Member

Choose a reason for hiding this comment

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

  1. Does it really make sense to check all these variants? What if I set ROCKSDB_STATIC=yes?
  2. Why do you use env::varwith .ok() rather than env::var_os ?
let mode = match env::var_os(&format!("{}_STATIC", lib_name))
            .as_ref()
            .and_then(|s| s.to_str()) {
...

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will change to var_os, I wasn't aware of the internal implementation of https://doc.rust-lang.org/src/std/env.rs.html#236 fn var.
Will also add yes as an enabling value.

The idea to accept only specific names or empty, is to allow the inverse usage as well: ROCKSDB_STATIC=false is something that is intuitive, when ROCKSDB_STATIC=1 enables static linkage.

@aleksuss
Copy link
Member

Also adds the option to NOT link statically if i.e. ROCKSDB_STATIC=false is provided

If you don't want to link statically just unset env variable. So. I'd leave logic of working with env variable ROCKSDB_STATIC as is.
cc @stanislav-tkach

@drahnr
Copy link
Contributor Author

drahnr commented Oct 17, 2022

I did a brief survey, and the dependencies are less coherent than I expect, tbh it's a mess.

https://github.com/rust-lang/libz-sys/blob/main/build.rs#L25
https://github.com/alexcrichton/xz2-rs/blob/master/lzma-sys/build.rs#L12
https://github.com/alexcrichton/bzip2-rs/blob/master/bzip2-sys/build.rs#L15-L24
https://github.com/10XGenomics/lz4-rs/blob/master/lz4-sys/build.rs#L43
https://github.com/gyscos/zstd-rs/blob/master/zstd-safe/zstd-sys/build.rs#L22-L25

Hence, I don't care about the solution, it'd be nice to have the duality that at least some references above exhibit in regards to value 1 or add a feature that ensures static compilation and linkage of all deps. The latter is out of scope, the former is debatable.

Note that with the current change, the current behavior of having an ROCKSB_STATIC="" resulting in static-linkage is preserved.

@stanislav-tkach
Copy link
Member

I think that changing the ROCKSDB_STATIC environment variable behavior is technically a breaking change.

@aleksuss aleksuss merged commit 83eea97 into rust-rocksdb:master Oct 21, 2022
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.

None yet

3 participants