Decouple library from platformio and refactor code

[N3xed]

First batch for #8.

• Refactor the pio code into its own module.
• Separate all code that is general purpose.
  • Refactor LinkArgs, CfgArgs, CInclArgs
• Test
• Change README

Sorry for spamming you with PRs. I'm creating this primarily that we can discuss things about the code, as well as to keep things more organized for me.

Note: This first commit does more than in my previous PR.

[ivmarkov]

This is already in a pretty good shape. Let me test it a bit tmr and if no big issues, I'll probably merge it. Renaming the crate itself and the other changes can be separate PRs.

[N3xed]

Ok, I've tested it and it compiles but I've run into some different problems when actually running the code (my test project, and the rust-esp32-example).

• Firstly, the libstd references the pthread_* primitives which are defined in esp-idf-sys, but because esp-idf-sys comes before libstd in the link command-line, this causes the linker to emit errors of undefined references to these symbols.
• Secondly, using EspMutex causes a LoadProhibited exception (LoadProhibited exception when using EspMutex esp-idf-sys#8).

But other than that, it compiles and links fine so it should be ready to be merged.

Renaming the crate itself and the other changes can be separate PRs.

I don't quite understand. Do you mean we should keep the package and library name as cargo-pio right now? Or do you mean we should rename the repository later? If it's the former, I don't see what the advantage is of that.

There's also the question if the cargo-pio binary should become its own package so that it can still be published to crates.io under that name. Because if the package is renamed to embuild and published to crates.io all its binaries will also be published and so will be installed when running cargo install embuild.

[ivmarkov]

Ok, I've tested it and it compiles but I've run into some different problems when actually running the code (my test project, and the rust-esp32-example).

• Firstly, the libstd references the pthread_* primitives which are defined in esp-idf-sys, but because esp-idf-sys comes before libstd in the link command-line, this causes the linker to emit errors of undefined references to these symbols.

As per our conversation in the channel, let's try to find out what really is causing this behavior. We might be uncovering an issue which was always there and so far worked only based on pure coincidence, or something has changed indeed.

• Secondly, using EspMutex causes a LoadProhibited exception (LoadProhibited exception when using EspMutex esp-rs/esp-idf-sys#8).

Hopefully I've fixed that now. Will check in more detail tmr.

But other than that, it compiles and links fine so it should be ready to be merged.

You mean, putting aside the pthread_rwlock_ errors?

Renaming the crate itself and the other changes can be separate PRs.

I don't quite understand. Do you mean we should keep the package and library name as cargo-pio right now? Or do you mean we should rename the repository later? If it's the former, I don't see what the advantage is of that.
I meant:

• Keeping the package name (temporarily) to "cargo-pio"
• Changing the library name to "embuild"
• Keeping the repo name (temporarily) to "cargo-pio"

(If that's possible.)

There's also the question if the cargo-pio binary should become its own package so that it can still be published to crates.io under that name. Because if the package is renamed to embuild and published to crates.io all its binaries will also be published and so will be installed when running cargo install embuild.

^^^ That's precisely the topic I was trying to not deal with as part of that first commit.

[N3xed]

• Keeping the package name (temporarily) to "cargo-pio"
• Changing the library name to "embuild"
• Keeping the repo name (temporarily) to "cargo-pio"

I think that works. You can at least name the library differently than the package. So the dependency name would remain the same but in the code, we would refer to embuild (if I'm interpreting that correctly rust-lang/cargo#6827).
But at some point, we would have (or should) rename the package either way, so that the dependency name and the lib name match again (caro-pio vs embuild). So I think we should just do it now and either:

• Extract both src/bin/cargo-pio.rs and src/bin/cargo-linkproxy.rs into their own folders and make them into (sub)crates themselves. These (sub)crates then would have to be published separately to crates.io in addition to embuild.
• Or make separate repos for them (which I think is overkill for now).

This would also mean that the user would have to install both cargo-pio and cargo-linkproxy (or I guess both at the same time). But I think that's not really an issue.

[N3xed]

Another question is if we shouldn't rename cargo-linkproxy to just linkproxy because it doesn't really have anything to do with cargo.

[ivmarkov]

Ok, I've tested it and it compiles but I've run into some different problems when actually running the code (my test project, and the rust-esp32-example).

• Firstly, the libstd references the pthread_* primitives which are defined in esp-idf-sys, but because esp-idf-sys comes before libstd in the link command-line, this causes the linker to emit errors of undefined references to these symbols.

As per our conversation in the channel, let's try to find out what really is causing this behavior. We might be uncovering an issue which was always there and so far worked only based on pure coincidence, or something has changed indeed.

OK so this one I figured out. TL;DR: It was working purely coincidentally, and has nothing to do with recent changes to rustc/cargo, and all to do with a change I did the other day to esp-idf-sys and esp-idf-svc. Putting this piece of code in rust-esp32-std-hello's main.rs/main fixes the issue (but of course I'll do a cleaner fix):

{
    let rwlock = std::sync::RwLock::new(false);
    let mut guard = rwlock.write().unwrap();
    *guard = true;
}

Long explanation:
You are completely right that only libstd, libcore & friends are currently surrounded by a --start-group / --end-group pairs, and I think the reason for this is that Rust maintainers do not expect circular references anywhere else:

• Regular crates are ordered according to the topological sorting of their dependency tree (I think), so in theory no issues here
• The circular refs amongst libstd, libcore and friends come primarily from language items.

The drama comes from the fact that I'm (still) using the esp-idf-sys crate as a "backfiller" of sorts for C-level APIs missing from ESP-IDF itself but used by libstd & friends. pthread-rwlock- is one example; another is the esp32c3 missing __atomic* libcalls. (Both of these will disappear from the ESP-IDF in the near future, so this problem is a temporary one and is OK to fix with a temporary workaround/hack.)

Now the reason why it did work before is because of this which I removed yesterday - which in turn was used in esp-idf-svc here. The removed code DID cause the pthread-rwlock- functions to get linked in because they were referenced not just by libstd, but also by our binary crate code, indirectly, via esp-idf-svc. (By the way, I suspect Rust's aggressive inlining also plays a role here, or else I don't see how the usage of pthread-rwlock- symbols by libstd's RwLock will be visible from our binary crate etc.)

Anyway. Declaring a pub static mut reference to pthread_rwlock_init inside esp-idf-sys seems to force Rust into keeping those symbols and the linkage errors go away. This is what I'll do as a temporary workaround shortly.

[ivmarkov]

• Keeping the package name (temporarily) to "cargo-pio"
• Changing the library name to "embuild"
• Keeping the repo name (temporarily) to "cargo-pio"

I think that works. You can at least name the library differently than the package. So the dependency name would remain the same but in the code, we would refer to embuild (if I'm interpreting that correctly rust-lang/cargo#6827).
But at some point, we would have (or should) rename the package either way, so that the dependency name and the lib name match again (caro-pio vs embuild). So I think we should just do it now and either:

• Extract both src/bin/cargo-pio.rs and src/bin/cargo-linkproxy.rs into their own folders and make them into (sub)crates themselves. These (sub)crates then would have to be published separately to crates.io in addition to embuild.
• Or make separate repos for them (which I think is overkill for now).

OK fine, let's bite the bullet. Could you - as part of this PR - create sub-crates for cargo-pio and cargo-linkproxy?
I would assume, only the main.rs code of both will be in the sub-crates?
LinkArgs and the pio module and sub-modules will remain in embuild?

This would also mean that the user would have to install both cargo-pio and cargo-linkproxy (or I guess both at the same time). But I think that's not really an issue.

It is a bit annoying though. And is here to stay, because of cargo pio espidf menuconfig. Even if we have tomorrow a different/additional command to invoke the ESP-IDF menuconfig functionality (say, cargo idf ... or cargo espidf ...) users still will have to install and keep updated the linkproxy in addition to the espidf subcommand. That's why the linkerproxy is currently distributed and auto-installed when you install the cargo-pio subcommand (and its name, cargo-pio-link is carefully chosen to avoid name collisions as much as possible).

See why I was hesitant to have these things implemented immediately? Feature creep and the need to take decisions on stuff that we might be better prepared to address later (i.e. when cargo idf/espidf sees the light of the day or at least we know how it is going to look like.)

Oh well. I guess if too inconvenient for users, we can always restore the linkproxy inside the upcoming cargo-pio sub-crate, and also inside the cargo-idf sub-crate, whenever that one sees the light of the day.

[ivmarkov]

Another question is if we shouldn't rename cargo-linkproxy to just linkproxy because it doesn't really have anything to do with cargo.

I'm fine with that. Users will never have to invoke the linkproxy command by themselves, so it can be named however we decide. It was named - as per above - cargo-pio-linkproxy to avoid name collisions, as it was distributed as part of cargo-pio.

[N3xed]

This would also mean that the user would have to install both cargo-pio and cargo-linkproxy (or I guess both at the same time). But I think that's not really an issue.

It is a bit annoying though. And is here to stay, because of cargo pio espidf menuconfig. Even if we have tomorrow a different/additional command to invoke the ESP-IDF menuconfig functionality (say, cargo idf ... or cargo espidf ...) users still will have to install and keep updated the linkproxy in addition to the espidf subcommand. That's why the linkerproxy is currently distributed and auto-installed when you install the cargo-pio subcommand (and its name, cargo-pio-link is carefully chosen to avoid name collisions as much as possible).

See why I was hesitant to have these things implemented immediately? Feature creep and the need to take decisions on stuff that we might be better prepared to address later (i.e. when cargo idf/espidf sees the light of the day or at least we know how it is going to look like.)

Oh well. I guess if too inconvenient for users, we can always restore the linkproxy inside the upcoming cargo-pio sub-crate, and also inside the cargo-idf sub-crate, whenever that one sees the light of the day.

I thought about it some more, and I think separating linkproxy and cargo-pio is the way forward. linkproxy is general enough to also be in other projects without cargo-pio or the espidf. Also when cmake support lands on the esp-idf-sys (which all these changes are really for) then cargo-pio is only needed for pio support, otherwise the user only has to install linkproxy. And additionally when we have a general tool like cargo-espidf which functions like idf.py the user can optionally install that too.

Finally, if it's really too much to install two crates then like you said, we could just add this bin to either cargo-pio and/or cargo-espidf.

[ivmarkov]

This would also mean that the user would have to install both cargo-pio and cargo-linkproxy (or I guess both at the same time). But I think that's not really an issue.

It is a bit annoying though. And is here to stay, because of cargo pio espidf menuconfig. Even if we have tomorrow a different/additional command to invoke the ESP-IDF menuconfig functionality (say, cargo idf ... or cargo espidf ...) users still will have to install and keep updated the linkproxy in addition to the espidf subcommand. That's why the linkerproxy is currently distributed and auto-installed when you install the cargo-pio subcommand (and its name, cargo-pio-link is carefully chosen to avoid name collisions as much as possible).
See why I was hesitant to have these things implemented immediately? Feature creep and the need to take decisions on stuff that we might be better prepared to address later (i.e. when cargo idf/espidf sees the light of the day or at least we know how it is going to look like.)
Oh well. I guess if too inconvenient for users, we can always restore the linkproxy inside the upcoming cargo-pio sub-crate, and also inside the cargo-idf sub-crate, whenever that one sees the light of the day.

I thought about it some more, and I think separating linkproxy and cargo-pio is the way forward. linkproxy is general enough to also be in other projects without cargo-pio or the espidf. Also when cmake support lands on the esp-idf-sys (which all these changes are really for) then cargo-pio is only needed for pio support, otherwise the user only has to install linkproxy. And additionally when we have a general tool like cargo-espidf which functions like idf.py the user can optionally install that too.

Finally, if it's really too much to install two crates then like you said, we could just add this bin to either cargo-pio and/or cargo-espidf.

OK. As I said - let's bite the bullet. :)
Will you be extending this patch to include these two sub-crates as well as the rename of the package? Or shall I do this?

[ivmarkov]

(I can rename the repo myself once these changes land.)

[N3xed]

Will you be extending this patch to include these two sub-crates as well as the rename of the package? Or shall I do this?

Yup, I'm on it right now.

[N3xed]

Okay, I've created the sub-crates, moved the current README into the cargo-pio folder, and added some pretty barebones READMEs for the other crates. I've also changed some keywords and categories.

I also want to know what you think about renaming linkproxy to ldproxy. I thought linkproxy would be pretty confusing (ie. proxy=proxy-server, link=url) so I changed the name to ldproxy.

[ivmarkov]

Okay, I've created the sub-crates, moved the current README into the cargo-pio folder, and added some pretty barebones READMEs for the other crates. I've also changed some keywords and categories.

I also want to know what you think about renaming linkproxy to ldproxy. I thought linkproxy would be pretty confusing (ie. proxy=proxy-server, link=url) so I changed the name to ldproxy.

ldproxy might also not be a perfect name, because ld is just one linker. Are we going to support only ld, forever? And in fact, we are proxying gcc, not ld directly (gcc is then proxying ld for us).

But.. no big deal! I'm fine with ldproxy as well.

[N3xed]

And in fact, we are proxying gcc, not ld directly (gcc is then proxying ld for us).

Yeah, that's too bad. But I think its better than linkproxy at least.

Okay, so I think this should be good enough right now to be merged.
Tell me if you want something to be changed or improved before that.