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

More context on a cranelift-codegen build script error #9625

Closed
arihant2math opened this issue Nov 20, 2024 · 5 comments · Fixed by #9633
Closed

More context on a cranelift-codegen build script error #9625

arihant2math opened this issue Nov 20, 2024 · 5 comments · Fixed by #9633

Comments

@arihant2math
Copy link

arihant2math commented Nov 20, 2024

Currently if an explicit isle directory is found when the "isle-in-source-tree" feature is not enabled, the following error message is shown by the build script for cranelift-codegen:

 --- stderr
  Error: directory isle_generated_code/ exists but is only used when
  `--feature isle-in-source-tree` is specified. To prevent confusion,
  this build script requires the directory to be removed when reverting
  to the usual generated code in target/. Please delete the directory and
  re-run this build.

as emitted by https://github.com/bytecodealliance/wasmtime/blob/main/cranelift/codegen/build.rs#L78C9-L87C10.

However this is misleading as deleting the target/ directory (atleast on windows) does not fix the issue. The directory, for me, was located in %HOME%\\.cargo\\registry\\src\\index.crates.io-6f17d22bba15001f\\cranelift-codegen-0.113.1\\isle_generated_code. Could there possibly be a better check/error message and/or the actual directory located printed out to stderr?

@cfallin
Copy link
Member

cfallin commented Nov 20, 2024

This seems to be a duplicate of #9588. That directory should not ever be created inside the .cargo path. Could you say more about how you're building Cranelift?

@arihant2math
Copy link
Author

I'm using wasmtime, so building is done because/through that. I'm not using any special features, just the default configuration.

I did some digging around the code, I think the bug is around L61 when calling env::current_dir(), since I'm pretty sure that's inconsistent when used in build scripts, I printed it out and got "%HOME%\.cargo\registry\src\index.crates.io-6f17d22bba15001f\cranelift-codegen-0.113.1", which seems to be the issue.

If changed to the OUT_DIR env variable, it would be in the target folder, which would be an improvement.

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 20, 2024

env::current_dir() is used for exactly two purposes in the build script:

  • Getting the source files. Using OUT_DIR here would be wrong.
  • Looking for a isle_generated_code directory in the source directory to use if isle-in-source-tree is enabled. The very intent of isle-in-source-tree requires this to keep using the source directory too.

If you aren't doing development on Cranelift itself, isle-in-source-tree should never be enabled (and it definitively shouldn't be enabled when using it as crates.io dependency as those are not allowed to write to their source directory) and as such isle_generated_code should never be generated in the source directory. We want to know why it is generated anyway. Is any project you built that depends on Cranelift using isle-in-source-tree?

@bjorn3
Copy link
Contributor

bjorn3 commented Nov 20, 2024

So turns out until very recently Wasmer was enabling isle-in-source-tree: wasmerio/wasmer#5202 Did you build Wasmer within the past couple of weeks? Note that Wasmer and Wasmtime are two entirely separate wasm engines. They just happen to share Cranelift as one of the compiler backends they use.

@cfallin
Copy link
Member

cfallin commented Nov 20, 2024

That's fairly obnoxious. Perhaps we should remove the feature altogether, unless folks are still using it -- I'll bring this up in the Cranelift meeting in a few minutes.

cfallin added a commit to cfallin/wasmtime that referenced this issue Nov 20, 2024
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.
github-merge-queue bot pushed a commit that referenced this issue 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.
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 a pull request may close this issue.

3 participants