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

Cranelift: rework isle-in-source-tree functionality. #9633

Merged

Conversation

cfallin
Copy link
Member

@cfallin cfallin commented Nov 20, 2024

Per #9625 and #9588, a downstream user of Cranelift was misusing the isle-in-source-tree feature, enabling it indiscriminately even in published crates (wasmerio/wasmer#5202). This went against the intent of the feature, to be a way for local developers to observe the generated source. As such, it ran afoul of some safety checks for that purpose, and also polluted users' Cargo caches in a way that we cannot now fix except in future releases.

The best we can do is probably to take away features that are liable to be misused. Following discussion in today's Cranelift weekly meeting, we decided to provide this functionality under an environment variable instead. This allows folks who know what they're doing to get the same behavior, but does not expose a feature to crate users.

Fixes #9625.

Per bytecodealliance#9625 and bytecodealliance#9588, a downstream user of Cranelift was misusing the
`isle-in-source-tree` feature, enabling it indiscriminately even in
published crates (wasmerio/wasmer#5202). This went against the intent of
the feature, to be a way for local developers to observe the generated
source. As such, it ran afoul of some safety checks for that purpose,
and also polluted users' Cargo caches in a way that we cannot now fix
except in future releases.

The best we can do is probably to take away features that are liable to
be misused. Following discussion in today's Cranelift weekly meeting, we
decided to provide this functionality under an environment variable
instead. This allows folks who know what they're doing to get the same
behavior, but does not expose a feature to crate users.

Fixes bytecodealliance#9625.
@cfallin cfallin requested a review from a team as a code owner November 20, 2024 17:20
@cfallin cfallin requested review from abrown and removed request for a team November 20, 2024 17:20
@cfallin cfallin enabled auto-merge November 20, 2024 17:26
@cfallin cfallin added this pull request to the merge queue Nov 20, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2024
@cfallin cfallin added this pull request to the merge queue Nov 20, 2024
@cfallin
Copy link
Member Author

cfallin commented Nov 20, 2024

I think the merge queue run raced with the release; cargo-vet failed because it found a version on crates.io it didn't expect. Re-running, but noting in case we see it again later -- interesting corner case!

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 20, 2024
@alexcrichton
Copy link
Member

Ah I don't think this is spurious but should be fixed at #9635

@alexcrichton alexcrichton added this pull request to the merge queue Nov 20, 2024
Merged via the queue into bytecodealliance:main with commit b502cf9 Nov 20, 2024
40 checks passed
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.

More context on a cranelift-codegen build script error
2 participants