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

Remove build script #490

Closed

Conversation

GuillaumeGomez
Copy link
Member

Fixes #489.

This is the implementation suggested in #489. Another possibility to reduce the number of cfg was to use macros but since we're talking about improving build times, I preferred to not use them.

src/lib.rs Outdated
AtomicUsize { v: Cell::new(v) }
}
// In case it has atomics. See https://github.com/rust-lang/log/issues/489.
#[cfg(not(all(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg(not(all(
#[cfg(not(any(

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch!

src/lib.rs Outdated
if current == prev {
self.v.set(new);
// In case it has atomic_cas.
#[cfg(any(
Copy link
Member

Choose a reason for hiding this comment

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

This duplicates the outer module's cfg

Copy link
Member Author

Choose a reason for hiding this comment

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

No it doesn't, the difference is the first one:

thumbv4t-none-eabi vs thumbv6m-none-eabi

I didn't want to only keep this one so it would make it easier for potential future updates.

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

LGTM.

Out of curiosity did you measure the build time differences?

Note for other reviews: you might want to enable GitHub's "Hide whitespace" feature (File changed > cog icon below it > Hide whitespace).

@GuillaumeGomez
Copy link
Member Author

I didn't for the simple reason I think there is close to none (it builds far too quickly on my computer to be able to measure a difference). Like said in the issue, its impact will be on the global rust environment.

@GuillaumeGomez GuillaumeGomez force-pushed the remove-build-script branch 3 times, most recently from 5603c43 to 64fa754 Compare March 4, 2022 15:30
@GuillaumeGomez
Copy link
Member Author

So the current failure is because there is no target and there is apparently no way to get an equivalent with the default taget_* provided:

$ rustc --print cfg --target thumbv4t-none-eabi
debug_assertions
panic="abort"
target_abi="eabi"
target_arch="arm"
target_endian="little"
target_env=""
target_feature="thumb-mode"
target_has_atomic_equal_alignment="16"
target_has_atomic_equal_alignment="32"
target_has_atomic_equal_alignment="8"
target_has_atomic_equal_alignment="ptr"
target_has_atomic_load_store="16"
target_has_atomic_load_store="32"
target_has_atomic_load_store="8"
target_has_atomic_load_store="ptr"
target_os="none"
target_pointer_width="32"
target_vendor="unknown"

As you can see, there is no equivalent of thumbv4t, which is problematic. My best bet at this point is trying to generate a cfg by using the env! macro somehow.

@GuillaumeGomez
Copy link
Member Author

I think it's not possible to use env! in a cfg. So unfortunately, I think there is no way to get rid of the build.rs file here...

@GuillaumeGomez GuillaumeGomez deleted the remove-build-script branch March 4, 2022 17:24
@GuillaumeGomez GuillaumeGomez mentioned this pull request Mar 4, 2022
}
// In case it has atomics. See https://github.com/rust-lang/log/issues/489.
#[cfg(not(any(
target = "thumbv4t-none-eabi",

This comment was marked as resolved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but I can't find the thumbv4t information in any of them as mentionned here.

@nnethercote
Copy link

So the current failure is because there is no target and there is apparently no way to get an equivalent with the default target_* provided:

Sorry, I don't understand this. Can you explain some more? What is the current failure?

BTW, when I run rustc --print cfg --target thumbv4t-none-eabi with nightly I get the same output that you got above, but when I run with Rust 1.57.0 I get this:

[gulf:~] rustc --print cfg --target thumbv4t-none-eabi
debug_assertions
target_arch="arm"
target_endian="little"
target_env=""
target_os="none"
target_pointer_width="32"
target_vendor="unknown"

I don't know if this is significant.

@GuillaumeGomez
Copy link
Member Author

We can get the target_os ("none") and the target_abi ("eabi") but I can't find how to get "thumbv4t". Also in your case, it seems like you don't even have the target_abi. It's a bit weird...

@nnethercote
Copy link

Please bear with me as I get my head around this. Are you saying that this code:

#[cfg(any(
    target = "thumbv4t-none-eabi",
    target = "msp430-none-elf",
    target = "riscv32i-unknown-none-elf",
    target = "riscv32imc-unknown-none-elf",
))] 

doesn't work? How does it fail? Is it a compile-time problem, a run-time problem, something else?

@GuillaumeGomez
Copy link
Member Author

It doesn't work because target doesn't exist, making the cfg always invalid.

@nnethercote
Copy link

I see, I thought target already existed. rust-lang/rfcs#2991 is open to add it.

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.

Remove build.rs
5 participants