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

Resolve log ambiguity #518

Open
nullstalgia opened this issue Nov 21, 2024 · 7 comments
Open

Resolve log ambiguity #518

nullstalgia opened this issue Nov 21, 2024 · 7 comments

Comments

@nullstalgia
Copy link

Howdy. I've been using a lot from esp-rs's work recently, and I've been really impressed! The traits from embedded-hal let me move a device from native SPI to bitbanged SPI, and use the original esp-idf-hal impls as a reference, worked almost first try!.

Back on topic, I wanted to mention something that's disappointed me and I think needs addressed better than it has been before.

image

There's a module in here named log, and there's an external dependency named log.

This leads to confusion in cases like this one:

use crate::sys::*;

use log::warn;

Whenever the compiler can't fully reason out which log to use (::log: the crate, or sys::log: the module), it halts compilation and throws that up.

I'm writing this now because of this happening again in fatfs.rs.

While I could just throw in a PR that uses ::log in that file, that feels inadequate. I also don't want to alias the log macros in Cargo.toml with

logm = { package = "log", ...

or try to rename the log module without consulting with y'all, so here I am.

My naïve suggestion is to rename the module to perhaps logger, given that's how it's sometimes referred to in that small file.

What do you think is the best solution?

@ivmarkov
Copy link
Collaborator

Yes, naming the esp_idf_svc::log module log was a mistake we did early on.

However, now is too late - every single example starts with this line early-on in main and renaming esp_idf_svc::log to something else means we'll just break the world.

If there are resolution issues within some concrete module of the esp-idf-* crates - like as you say - fatfs, these need to be resolved within that module these days.

Btw, glob imports (use esp_idf_svc::sys::*) that we lazily use everywhere aren't helping either.

What compile error do you see in the fatfs module exactly?

@nullstalgia
Copy link
Author

Fair enough about wanting to keep existing examples compatible.

Yeah, the globs definitely don't help, but I also don't know if the benefit of denying them is more than keeping them for dev UX. That's up to you guys.

As for the error I get:

(The netif one only is on master, not the rev I was working with)

error[E0659]: `log` is ambiguous
  --> /home/name/.cargo/git/checkouts/esp-idf-svc-a28457b0e32c6283/9e70526/src/fs/fatfs.rs:7:5
   |
7  | use log::warn;
   |     ^^^ ambiguous name
   |
   = note: ambiguous because of a conflict between a name from a glob import and an outer scope during import or macro resolution
   = note: `log` could refer to a crate passed with `--extern`
   = help: use `::log` to refer to this crate unambiguously
note: `log` could also refer to the struct imported here
  --> /home/name/.cargo/git/checkouts/esp-idf-svc-a28457b0e32c6283/9e70526/src/fs/fatfs.rs:10:5
   |
10 | use crate::sys::*;
   |     ^^^^^^^^^^^^^
   = help: consider adding an explicit import of `log` to disambiguate
   = help: or use `self::log` to refer to this struct unambiguously

error[E0659]: `log` is ambiguous
   --> /home/tony/.cargo/git/checkouts/esp-idf-svc-a28457b0e32c6283/9e70526/src/netif.rs:963:9
    |
963 |     use log::debug;
    |         ^^^ ambiguous name
    |
    = note: ambiguous because of a conflict between a name from a glob import and an outer scope during import or macro resolution
    = note: `log` could refer to a crate passed with `--extern`
    = help: use `::log` to refer to this crate unambiguously
note: `log` could also refer to the struct imported here
   --> /home/name/.cargo/git/checkouts/esp-idf-svc-a28457b0e32c6283/9e70526/src/netif.rs:966:9
    |
966 |     use crate::sys::*;
    |         ^^^^^^^^^^^^^
    = help: consider adding an explicit import of `log` to disambiguate
    = help: or use `self::log` to refer to this struct unambiguously

For more information about this error, try `rustc --explain E0659`.
error: could not compile `esp-idf-svc` (lib) due to 2 previous errors

@ivmarkov
Copy link
Collaborator

What is really weird is that neither I nor the CI is seeing these errors. Do you enable some extra components in ESP IDF? Or do you have some special sdkconfig.defaults?

Trying to understand what defines the esp_idf_sys::bindings::log symbol, which seems to be the conflict.

@nullstalgia
Copy link
Author

nullstalgia commented Nov 21, 2024

It looks like the sdkconfig.defaults line CONFIG_BT_NIMBLE_ENABLED=y is what specifically is causing it, out of the lines I added:

CONFIG_BT_ENABLED=y
CONFIG_BT_BLE_ENABLED=y
CONFIG_BT_BLUEDROID_ENABLED=n
CONFIG_BT_NIMBLE_ENABLED=y # <- 

Plus I have the experimental feature on.

@ivmarkov
Copy link
Collaborator

CI does run with experimental enabled, but not with NimBLE enabled, so that's why we did not catch it.

Oh well, I think it would be easier if (once again) we search &replace use log::XXX with use ::log::XXX, rather than fixing the esp_idf_sys glob-imports everywhere.

Would you we willing to try it out by first addressing the compilation errors in the modules where you exhibit the problem (netif and fatfs)?

BTW: I assume all of that is with esp-idf-svc master?

@nullstalgia
Copy link
Author

nullstalgia commented Nov 22, 2024

There is the possibility of aliasing the log package to help prevent this in the future. I'd rather have a smart Clippy lint, but that requires more nightly shenanigans that I'm unfamiliar with. My main goal with this issue is wanting to prevent more devs like myself to have to open another issue/PR in a year or so when someone else forgets two colons and it slips through CI.

I originally was running with this commit, but master is also affected (if not more since I didn't have the netif error on that commit.)

And I'm already trying it out, seems to work fine, but I'm having issues with other SPI devices/threads (you might catch me in the matrix channel 😅) so I haven't quite been able to do more than read a sub-kb .txt from the cards.

ivmarkov added a commit that referenced this issue Nov 22, 2024
@ivmarkov
Copy link
Collaborator

In the meantime this is fixed with 3f2ce04

If you would like to work on a more elaborate solution, we can keep this issue open. But otherwise I would close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

No branches or pull requests

2 participants