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 to set environment variables from build script #114

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

eerii
Copy link

@eerii eerii commented Nov 14, 2024

system-deps provides many environment variables to configure its options. This is great for end users, but it doesn't allow libraries to provide defaults or configure them programmatically.

An improvement could be to have a add_env method in the build script configuration that will add certain values to its environment (it doesn't set the real env variable). Since system-deps already had a Mock variable type tests, we can reuse it, providing a fallback to the environment in the case of build scripts and not doing so in tests.

Use cases

  • Configuring system-deps options using crate features. For example, a crate could have a feature that determines if it is statically or dynamically linked, and using add_env we can set the relevant variable from the build script.
  • Allow dependencies to provide defaults, for example, set linking line deduplication if the crate requires it.
  • Better interface with other env variables specific to the crate. If a package already reads the option SOME_CONFIG, it can now also set needed system-deps variables.

Considered alternatives

  • Use .cargo/config.toml to set [env] variables per target. However, rust doesn't allow to use features to configure these values, and dependencies can't share this with users.
  • Build scripts can emit environment variables and metadata using cargo directives. However, the first is only passed to it's corresponding library, and the second only sends the metadata to its dependent crates, not its dependencies (like system-deps).
  • Users can set the variables directly or with a Makefile/similar script. However, this requires extra configuration when using a dependency and it can cause unexpected issues.

TODO:

  • Should the environment or the set variables have priority? I'm leaning towards the current implementation (set variables shadow environment) since then the decision lays on the crate using system-deps. They can read the environment and decide whether to use it or not.
  • Add tests.
  • Link related PRs.

Note: This was originally part of the PR for supporting prebuilt binaries (ADD LINK). However, in an effort to make reviewing easy, I separated this feature since it works independently.

@gdesmott
Copy link
Owner

When @sdroege and I designed system-deps env variables were meant to be used only by downstream/distribution in case they need to override things when integrating with their system.
I still think this was a sane design choice, so I'd rather stick to it unless we have a very good reason not to.

So maybe we should first look at each actual use case you have for this and consider alternatives?

@sdroege: what do you think?

@eerii
Copy link
Author

eerii commented Nov 14, 2024

That seems very reasonable. During development I used this in a few places. However, after all of the refactoring, the only reason I still used this was to enforce the clean linking variable without requiring the user to specify it. But if we actually do the deduplication correctly we don't need the variable in the first place.

It is true that I could see other some potential uses for this (that's why I kept it in), like setting the environment based on features (since cargo doesn't allow this yet). However, if this was an intentional design decision, the best thing would be to do as you say and see if there are real use cases that can't be solved otherwise.

I will take note of the downstream part to make an addendum to the prebuilt binaries proposal (I'm almost done, it's taken a while to write everything up) that I think better respects this.

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.

2 participants