abrown commented on issue #3821:
Some more details on this change:
- I wouldn't mind a second look at what was necessary to implement the multi-OS support in
ittapi-rs
; e.g., right now there are bindings built for each supported OS but unsupported OS-es will cause a build failure because they export nothing (see thelib.rs
file)- I ran
cargo build --release
before and after this change on a Fedora machine: without thevtune
feature, the binary was 18,674,952 bytes; withvtune
on by default, the binary was 18,691,768 bytes. That seemed like an appropriately-small increase.- Thanks to @bnjbvr and @jlb6740 for working on this as well!
github-actions[bot] commented on issue #3821:
Subscribe to Label Action
cc @peterhuene
<details>
This issue or pull request has been labeled: "wasmtime:api", "wasmtime:docs"Thus the following users have been cc'd because of the following labels:
- peterhuene: wasmtime:api
To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.
Learn more.
</details>
alexcrichton commented on issue #3821:
In addition to what @bnjbvr mentioned to make this default I think this block and this block need updating to include
vtune
abrown commented on issue #3821:
cfg[target_os = "linux"] that need to be tweaked in profiling.rs iirc
This is resolved by the latest commit.
we can remove it from the list of dependencies (even optional), and then have per-os specific dependencies that both add the dependency and enable the feature
We could, yes, but it seems like we should do one or the other: either enable the
vtune
feature exclusively in Cargo like you suggest OR have thecfg(...)
conditions in the code itself. This could be resolved in a later PR, I think.I think this block and this block need updating to include vtune
One of those was already "defaulted" by this PR but I added a default to
wasmtime-jit
to be consistent.
abrown commented on issue #3821:
Oh, hold on: we also need to check for
x86_64
incfg(...)
as well, right?
alexcrichton commented on issue #3821:
Oh reading more closely ... personally I think it's ok to skip platform-specific checks in the #[cfg] because Wasmtime probably only really compiles on Linux/macOS/Windows anyway and other platforms likely hit compile errors elsewhere so this wouldn't be the only issue. If Wasmtime is ported to other platforms I think it'd be good to leave this in as "hey this crate probably needs an update" if possible
abrown commented on issue #3821:
@bnjbvr, @alexcrichton: this removes the conditional compilation for the OS based on @alexcrichton's comment--this way, if someone ports Wasmtime to another OS but does not turn the
vtune
feature off, they will get a compile error inittapi-rs
which will motivate adding the OS support there.I did add a
target_arch = "x86_64"
check since this feature only makes sense to be enabled on those machines anyways.ittapi-rs
seems to compile fine on other architectures so there should be no problem there but this makes it more clear what environment thevtune
feature is intended for.
Last updated: Dec 23 2024 at 13:07 UTC