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

Add SAW_RUST_LIBRARY_PATH alias for CRUX_RUST_LIBRARY_PATH #46

Merged
merged 2 commits into from
May 5, 2023

Conversation

RyanGlScott
Copy link
Contributor

@RyanGlScott RyanGlScott commented May 3, 2023

Fixes #43.

@RyanGlScott RyanGlScott requested review from spernsteiner and m10f May 3, 2023 13:36
@spernsteiner
Copy link
Collaborator

I think we should have some kind of prefix on our variable names so there's less risk of name collisions with other tools. This especially applies to the internal / non-user-facing vars ALREADY_SET_PATH, EXPORT_ALL, and USE_OVERRIDE_CRATES. Maybe CRUCIBLE_RUST_LIBRARY_PATH for the one user-facing var (it being the path to the crucible-compatible Rust libraries, and Crucible being the common denominator between Crux and SAW), and CRUX_* for the rest by historical precedent? Or we could allow both CRUX_RUST_LIBRARY_PATH and SAW_RUST_LIBRARY_PATH, so it makes sense for both tools.

@RyanGlScott
Copy link
Contributor Author

I'd be fine with CRUCIBLE_RUST_LIBRARY_PATH and keeping the rest unchanged. I'll push a new version which does this.

@RyanGlScott
Copy link
Contributor Author

Hm. Even if we rename it to CRUCIBLE_RUST_LIBRARY_PATH, there is an additional awkward tension in that all of the relevant .rlib/.mir files currently live in crux-mir. I suppose that when we distribute SAW in the future, we will want to include these .rlib/.mir files in the SAW bindist so that they can easily be located.

You know what? I think you've convinced me to pick SAW_RUST_LIBRARY_PATH, as I think that name will be less confusing for people who don't know what Crucible is.

@RyanGlScott RyanGlScott changed the title Deprecate CRUX_ environment variables Add SAW_RUST_LIBRARY_PATH alias for CRUX_RUST_LIBRARY_PATH May 5, 2023
@RyanGlScott
Copy link
Contributor Author

I've overhauled this PR to (1) introduce SAW_RUST_LIBRARY_PATH as an alias, and (2) revise the documentation to make a clearer distinction between public environment variables like {CRUX,SAW}_RUST_LIBRARY_PATH and other environment variables that are meant to be internal to mir-json-rustc-wrapper.

@RyanGlScott RyanGlScott merged commit 3a621e1 into master May 5, 2023
@RyanGlScott RyanGlScott deleted the T43 branch May 5, 2023 22:46
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.

Rename CRUX_RUST_LIBRARY_PATH to RUST_LIBRARY_PATH
2 participants