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 selecting owned identifier memory representation via regular environment variable #1458

Open
jplatte opened this issue Jan 25, 2023 · 7 comments

Comments

@jplatte
Copy link
Member

jplatte commented Jan 25, 2023

Currently, it is only possible to select the memory representation (Box or Arc) for identifiers (docs) via a RUSTFLAGS --cfg setting. Changing RUSTFLAGS leads to the entire dependency chain being recompiled. As a cheaper solution, we can read an environment variable in ruma-commons build script, and emit

cargo:rustc-cfg=ruma_identifiers_storage=ENV_VARIABLE_VALUE
cargo:rerun-if-env-changed=ENV_VARIABLE_NAME

(docs)

Then when people change the environment variable, only ruma-common and its dependents will be rebuilt.

@ShadowJonathan
Copy link
Member

Doesn't this require a build.rs?

@jplatte
Copy link
Member Author

jplatte commented Feb 2, 2023

Yes, is that problematic?

@ShadowJonathan
Copy link
Member

I don't think so, but just making sure

@HKalbasi
Copy link
Contributor

HKalbasi commented Feb 5, 2023

What is the benefit over using cargo features?

@jplatte
Copy link
Member Author

jplatte commented Feb 5, 2023

Cargo features are additive and can be enabled from any place in the dependency tree. For the owned identifier representation, one specific value must be set (it wouldn't make sense to activate multiple ones).

@HKalbasi
Copy link
Contributor

HKalbasi commented Feb 5, 2023

It can become a compiler error to activate multiple ones. This has potential to make some libraries unusable with each other, but I think in practice all libraries will delegate this decision to the main binary (specially when docs encourage that). I would prefer cargo features for better tooling support and less problems like this, but YMMV.

@ShadowJonathan
Copy link
Member

If it'll become a compiler error, then multiple libraries will become mutually exclusively incompatible with eachother, which is not what we want

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

No branches or pull requests

3 participants