only use env_logger in binaries

[tdelabro]

env_logger is used in the cairo-lang crates, a bit indiscriminately in libs and bin.

The crate documentation states that it should only be used in binaries. This PR makes sure it does.

The problem is that those binaries are part of the same crates as the lib they use, making them share the same [dependencies]. This is problematic because we don't want it imported at all while building the lib, but we want it when building the binary.

There is multiple ways to fix this, as explained in this StackOverflow answer. I chose the second option: defining a specific feature for this import and adding it as required-features in the [[bin]] section.

I did it because it is the less invasive solution for now. But my recommendation would be to separate entirely binaries from libs, by making them exist in a separate crate where we can have full control over their dependencies (it will be more handy when we start to support no_std builds).

Wdyt? Is it good enough for now? Do you prefer the standalone binary crate solutions? If so where should those crates exist, and how should they be named (<name of the lib>-bin? Something else?).

---

This change is 

[tdelabro]

@spapinistarkware Following the exchange you had with @abdelhamidbakhta about no_std support in the rust cairo libraries, I started working on it.
This is the first thing that will require a change (env_logger not being no_std compatible).

[spapinistarkware]

Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @tdelabro)

[spapinistarkware]

Looks good

…

On Tue, May 30, 2023, 22:07 Timothée Delabrouille ***@***.***> wrote: @spapinistarkware <https://github.com/spapinistarkware> Following the exchange you had with @abdelhamidbakhta <https://github.com/abdelhamidbakhta> about no_std support in the rust cairo libraries, I started working on it. This is the first thing that will require a change (env_logger not being no_std compatible). — Reply to this email directly, view it on GitHub <#3276 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKOAMHKM3SJ2SFAFAMKNRFTXIZAPJANCNFSM6AAAAAAYUNTLWY> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[spapinistarkware]

Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

---

scripts/cairo_fmt.sh line 3 at r2 (raw file):

#!/bin/bash

cargo run --bin cairo-format --features="binary" -- --recursive "$@"

Do I need to specify this when I run? Why is that, I thought defining in the toml is enough.
I'd really like to avoid this.

[tdelabro]

Yes, you will have to. The feature is required, meaning compilation won't even start if it's not there, but that does not mean that it is automatically used.

Two solutions:

• add a default = ["binary"] feature (but it will also be the default for the lib meaning that using default-features = false while importing the lib in another crate will be kinda mandatory)
• put the binaries in their own crates and all those shenanigans will not longer be required, because we can just import what need

Again I think in the long run, having the binaries in separate folders will be clearer and easier to maintain

The discussion about the functionality we would like to have are being hold here, with an RFC being redacted here

[mkaput]

Personally, I believe extracting binaries to a separate crate is a way to go upfront here. I suspect this play with features will bring issues to compiler crates users :/

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

[spapinistarkware]

@tdelabro Can you try to separate the binaries into different crate then?
Remember to add the new crate to the ci yml file.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

[tdelabro]

@spapinistarkware done.
I put all binaries under a crates/bin/ folder

[spapinistarkware]

Reviewed 29 of 29 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

[tdelabro]

@spapinistarkware should I rebase and you merge?
or do we want multiple reviewers?
Is there some specific policies on those repositories?

[spapinistarkware]

No need to rebase. I want @orizi to also tal, and then we'll merge.

[orizi]

Reviewed 2 of 9 files at r1, 2 of 8 files at r2, 27 of 29 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

[orizi]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

[tdelabro]

@spapinistarkware ok perfect

[mkaput]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @tdelabro)

---

crates/cairo-lang-language-server/src/lib.rs line 72 at r3 (raw file):

pub async fn serve_language_service() {
    init_logging(log::LevelFilter::Warn);

@maciektr please take a look at this, I think we will have to adapt Scarb's LS to also include logging, perhaps via tracing which we use there?