Remove `build.rs`

[nnethercote]

log's build.rs file is very simple. It just checks the target platform and then tells Cargo to pass in --cfg=atomic_cas and/or --cfg=has_atomics as necessary. Those conditions are then used in about 10 different places, all in src/lib.rs.

This use of build.rs is understandable. It's the only way to easily give a name to a condition, and these two conditions are somewhat verbose, checking against four targets for each one.

However, @lqd and I have recently been doing some large scale measurement and analysis of popular crates. We have learned several relevant things.

• log is a very popular crate, the 7th most commonly used in our data set.
• Build scripts, even tiny ones, have a non-trivial effect on compile times of projects, because they (a) take a small amount of time themselves to build and then run, and (b) more importantly, they add dependencies to the compilation graph.
• Build scripts, even tiny ones, can be quite large on disk, because they contain debug info from std. Around 4MB is typical on my Linux machine.
• log's build script is number 28 in the list of compile times across our entire analysis, even though it's so tiny. (log itself is number 23.)

Full data is available here.

With all this in mind, it seems worth eliminating log's build.rs file. To avoid repeating the target conditions multiple times, it would make sense to restructure the conditional code into modules. I haven't tried this myself, but just from looking at the code it doesn't look too hard. The change to do this should include a comment explaining why a build script is worth avoiding :smile:

This change will make little difference to the build times seen by the log developers, but would save a little compile time for everyone who develops code that depends on log. Across the entire Rust ecosystem, this will have a significant effect.

[nnethercote]

Note that there is work ongoing to make atomic feature detection possible directly with #[cfg(...)]: https://github.com/rust-lang/rust/issues/32976. However, it sounds like the ability to detect both the has_atomics case and the atomic_cas case would be some distance off, so in the meantime the approach suggested above is still worth doing. It would be worth an additional comment to say that these new cfg features could one day be used.

[GuillaumeGomez]

Sounds like a great improvement. I'll take a look.

[GuillaumeGomez]

So as mentionned in #490, it seems to be impossible.

• There is no equivalent of env!("TARGET") inside the various target_* (explained here.
• There is no way to enable feature based on a given target in Cargo.toml.
• We cannot use env! in a cfg.

The only option here would be to use a proc-macro. Would that be acceptable or is a build.rs preferrable?

[nnethercote]

The only option here would be to use a proc-macro. Would that be acceptable or is a build.rs preferrable?

A proc-macro would be a separate crate that must be built separately, just like the build script, so it would not be better.

[nagisa]

However, it sounds like the ability to detect both the has_atomics case and the atomic_cas case would be some distance off

cfg(target_has_atomic="ptr") is what this library calls atomic_cas (for usize) and is going to become stable shortly.

cfg(target_has_atomic_load_store="ptr") is the has_atomic, and is not currently on a path to stabilization. That said, I'm not entirely sure the distinction this library makes between atomic_cas and has_atomic is actually meaningful. This library's use of load and store is already racy in all cases I could find (set_max_level/max_level due to use of the Relaxed ordering, and set_logger_racy due to its implementation), and users are advised to turn off interrupts. Might as well replace the use of “atomics” in those situations with plain global variable holding an UnsafeCell or something on platforms without atomic_cas.

[nnethercote]

There was some discussion about this on Zulip. One recent comment:

Hm. It looks like log's build.rs is wrong in any case:

• atomic_cas is set for thumbv4t-none-eabi, but rustc claims CAS is not supported for that target: log probably doesn't compile today for thumbv4t-none-eabi
• target_has_atomics is false for thumbv4t-none-eabi, but the target does have atomics, just not CAS

@nnethercote so at least for thumbv4t-none-eabi it seems like making a change here is probably fine without a new major version of log, though someone should double check what I said -- maybe we can just break thumbv4t-none-eabi on older Rust compilers or something like that?

I know very little about these platforms that don't support atomics, but it does seem like the thread is worth reading. Perhaps removing the build.rs is still possible with the current state of cfg support?

[KodrAus]

In general, I kind of suspect the majority of library build scripts could be killed off with some expanded #[cfg]s to detect Rust versions and target features / reachability.

If we reach a point where we can remove the build script here I'd be 100% for it.

[KodrAus]

As of 1.60.0 we'd be able to start using #[cfg(target_has_atomic)] so once we're ready to bump to that we'll be able to remove this build script.