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

Fix build error with targets that do not support atomic CAS operations #12

Merged
merged 2 commits into from May 11, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 14 additions & 3 deletions .github/workflows/ci.yml
Expand Up @@ -26,9 +26,7 @@ jobs:
strategy:
matrix:
target:
# TODO: Currently, valuable cannot build with targets that do not have atomic CAS.
# We should port https://github.com/rust-lang/futures-rs/pull/2400.
# - thumbv6m-none-eabi
- thumbv6m-none-eabi
- thumbv7m-none-eabi
runs-on: ubuntu-latest
steps:
Expand Down Expand Up @@ -59,6 +57,19 @@ jobs:
# run: cargo install cargo-hack
# - run: cargo hack build --workspace --feature-powerset --no-dev-deps

# When this job failed, run ci/no_atomic_cas.sh and commit result changes.
# TODO(taiki-e): Ideally, this should be automated using a bot that creates
# PR when failed, but there is no bandwidth to implement it
# right now...
codegen:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v2
- name: Install Rust
run: rustup update nightly && rustup default nightly
- run: ci/no_atomic_cas.sh
- run: git diff --exit-code

fmt:
runs-on: ubuntu-latest
steps:
Expand Down
28 changes: 28 additions & 0 deletions ci/no_atomic_cas.sh
@@ -0,0 +1,28 @@
#!/bin/bash

# Update the list of targets that do not support atomic CAS operations.
#
# Usage:
# ./ci/no_atomic_cas.sh

set -euo pipefail
IFS=$'\n\t'

cd "$(cd "$(dirname "$0")" && pwd)"/..

file="valuable/no_atomic_cas.rs"

{
echo "// This file is @generated by $(basename "$0")."
echo "// It is not intended for manual editing."
echo ""
echo "const NO_ATOMIC_CAS_TARGETS: &[&str] = &["
} >"$file"

for target in $(rustc --print target-list); do
res=$(rustc --print target-spec-json -Z unstable-options --target "$target" \
| jq -r "select(.\"atomic-cas\" == false)")
[[ -z "$res" ]] || echo " \"$target\"," >>"$file"
done

echo "];" >>"$file"
41 changes: 41 additions & 0 deletions valuable/build.rs
@@ -0,0 +1,41 @@
#![warn(rust_2018_idioms, single_use_lifetimes)]

use std::env;

include!("no_atomic_cas.rs");

// The rustc-cfg listed below are considered public API, but it is *unstable*
// and outside of the normal semver guarantees:
//
// - `valuable_no_atomic_cas`
// Assume the target does *not* support atomic CAS operations.
// This is usually detected automatically by the build script, but you may
// need to enable it manually when building for custom targets or using
// non-cargo build systems that don't run the build script.
//
// With the exceptions mentioned above, the rustc-cfg strings below are
// *not* public API. Please let us know by opening a GitHub issue if your build
// environment requires some way to enable these cfgs other than by executing
// our build script.
Copy link
Member Author

@taiki-e taiki-e May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In futures, there was a question about compatibility with custom targets, so we exposed a similar cfg as an unstable public API.

We can also treat it as a private API until there is an actual request, but this cfg is unstable and we can change/remove it at any time, so I think it is also ok as-is. (Whether this cfg is considered private or public depends on how the comments describe about it, so either way, it is a very simple change.)

Btw, one reason for the instability is that this cfg can eventually be replaced by cfg(accessible). So I believe this cfg will never be a stable API.

Copy link
Member Author

@taiki-e taiki-e May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll need to implement similar detection logic for atomic integers later, so it's probably better to make this private for now. I'll update PR...

UPDATE: done in f7f1b95

fn main() {
let target = match env::var("TARGET") {
Ok(target) => target,
Err(e) => {
println!(
"cargo:warning=valuable: unable to get TARGET environment variable: {}",
e
);
return;
}
};

// Note that this is `no_*`, not `has_*`. This allows treating
// `cfg(target_has_atomic = "ptr")` as true when the build script doesn't
// run. This is needed for compatibility with non-cargo build systems that
// don't run the build script.
if NO_ATOMIC_CAS_TARGETS.contains(&&*target) {
println!("cargo:rustc-cfg=valuable_no_atomic_cas");
}

println!("cargo:rerun-if-changed=no_atomic_cas.rs");
}
11 changes: 11 additions & 0 deletions valuable/no_atomic_cas.rs
@@ -0,0 +1,11 @@
// This file is @generated by no_atomic_cas.sh.
// It is not intended for manual editing.

const NO_ATOMIC_CAS_TARGETS: &[&str] = &[
"avr-unknown-gnu-atmega328",
"msp430-none-elf",
"riscv32i-unknown-none-elf",
"riscv32imc-unknown-none-elf",
"thumbv4t-none-eabi",
"thumbv6m-none-eabi",
];
1 change: 1 addition & 0 deletions valuable/src/enumerable.rs
Expand Up @@ -213,6 +213,7 @@ impl<E: ?Sized + Enumerable> Enumerable for alloc::rc::Rc<E> {
}
}

#[cfg(not(valuable_no_atomic_cas))]
#[cfg(feature = "alloc")]
impl<E: ?Sized + Enumerable> Enumerable for alloc::sync::Arc<E> {
fn definition(&self) -> EnumDef<'_> {
Expand Down
1 change: 1 addition & 0 deletions valuable/src/listable.rs
Expand Up @@ -26,6 +26,7 @@ impl<L: ?Sized + Listable> Listable for alloc::rc::Rc<L> {
}
}

#[cfg(not(valuable_no_atomic_cas))]
#[cfg(feature = "alloc")]
impl<L: ?Sized + Listable> Listable for alloc::sync::Arc<L> {
fn size_hint(&self) -> (usize, Option<usize>) {
Expand Down
1 change: 1 addition & 0 deletions valuable/src/mappable.rs
Expand Up @@ -26,6 +26,7 @@ impl<M: ?Sized + Mappable> Mappable for alloc::rc::Rc<M> {
}
}

#[cfg(not(valuable_no_atomic_cas))]
#[cfg(feature = "alloc")]
impl<M: ?Sized + Mappable> Mappable for alloc::sync::Arc<M> {
fn size_hint(&self) -> (usize, Option<usize>) {
Expand Down
1 change: 1 addition & 0 deletions valuable/src/structable.rs
Expand Up @@ -108,6 +108,7 @@ impl<S: ?Sized + Structable> Structable for alloc::rc::Rc<S> {
}
}

#[cfg(not(valuable_no_atomic_cas))]
#[cfg(feature = "alloc")]
impl<S: ?Sized + Structable> Structable for alloc::sync::Arc<S> {
fn definition(&self) -> StructDef<'_> {
Expand Down
1 change: 1 addition & 0 deletions valuable/src/valuable.rs
Expand Up @@ -69,6 +69,7 @@ impl<V: ?Sized + Valuable> Valuable for alloc::rc::Rc<V> {
}
}

#[cfg(not(valuable_no_atomic_cas))]
#[cfg(feature = "alloc")]
impl<V: ?Sized + Valuable> Valuable for alloc::sync::Arc<V> {
fn as_value(&self) -> Value<'_> {
Expand Down