Fix target_vendor in non-IDF Xtensa ESP32 targets
#131170
Conversation
The Xtensa ESP32 targets are the following: - xtensa-esp32-none-elf - xtensa-esp32-espidf - xtensa-esp32s2-none-elf - xtensa-esp32s2-espidf - xtensa-esp32s3-none-elf - xtensa-esp32s3-espidf The ESP-IDF targets already set `target_vendor="espressif"`, however, the ESP32 is produced by Espressif regardless of whether using the IDF or not, so we should set the target vendor there as well.
|
r? @Nadrieril rustbot has assigned @Nadrieril. Use |
rustbot
added
S-waiting-on-review
|
These commits modify compiler targets. |
|
Hm. I don't think I've actually seen any discussion of it. |
|
r? compiler |
There was a problem hiding this comment.
The PR looks fine to me, whilst it's possible this CPU configuration could run on another vendors chip, it's unlikely, so adding Espressif as the vendor makes sense.
Digression: The targets are not really following the usual naming scheme, should probably have been named e.g. xtensa_esp32s2-espressif-none-elf? Or perhaps they should be combined into one target xtensa-espressif-none-elf, and then the user should set -Ctarget-cpu=esp32s2. But that's probably been discussed elsewhere, so I won't belabour the point.
I would tend to agree, but this is also an issue with other targets too, such as riscv, where we have many targets with various CPU features enabled (imc, imac, etc). There hasn't really been a policy in place (that I was aware of) for this. If we go to the effort to changing the Xtensa targets, I believe there should be a policy decision that goes with it to change the riscv or other targets too (note that some of these are tier 2 already, whether that affects anything - I'm not sure the target policy addresses a target rename). Given that Espressif don't plan to make any more Xtensa based MCUs, I don't feel like this is too bad in it's current form.
|
The discussion about target vs. target-cpu can be pursued elsewhere. I agree that it should probably be mostly considered as part of making a larger policy change, rather than a burden specifically on the Espressif maintainers. I am going to take the assent of the maintainer as sufficient for this change. While I am not a compiler team member, formally, I seem to have been baton-passed such authority when it comes to reviewing targets enough times. So: @bors r+ |
bors
added
S-waiting-on-bors
|
@bors rollup=always |
…r=workingjubilee Fix `target_vendor` in non-IDF Xtensa ESP32 targets `rustc`'s Xtensa ESP32 targets are the following: - `xtensa-esp32-none-elf` - `xtensa-esp32-espidf` - `xtensa-esp32s2-none-elf` - `xtensa-esp32s2-espidf` - `xtensa-esp32s3-none-elf` - `xtensa-esp32s3-espidf` The ESP-IDF targets already set `target_vendor="espressif"`, however, the ESP32 is, from my understanding, produced by Espressif regardless of whether using the IDF or not, so we should set the target vendor there as well?
…llaumeGomez Rollup of 4 pull requests Successful merges: - rust-lang#130824 (Add missing module flags for `-Zfunction-return=thunk-extern`) - rust-lang#131170 (Fix `target_vendor` in non-IDF Xtensa ESP32 targets) - rust-lang#131369 (Update books) - rust-lang#131370 (rustdoc: improve `<wbr>`-insertion for SCREAMING_CAMEL_CASE) r? `@ghost` `@rustbot` modify labels: rollup
Rollup of 7 pull requests Successful merges: - rust-lang#130824 (Add missing module flags for `-Zfunction-return=thunk-extern`) - rust-lang#131170 (Fix `target_vendor` in non-IDF Xtensa ESP32 targets) - rust-lang#131355 (Add tests for some old fixed issues) - rust-lang#131369 (Update books) - rust-lang#131370 (rustdoc: improve `<wbr>`-insertion for SCREAMING_CAMEL_CASE) - rust-lang#131379 (Fix utf8-bom test) - rust-lang#131385 (Un-vacation myself) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#131170 - madsmtm:target-info-esp32-vendor, r=workingjubilee Fix `target_vendor` in non-IDF Xtensa ESP32 targets `rustc`'s Xtensa ESP32 targets are the following: - `xtensa-esp32-none-elf` - `xtensa-esp32-espidf` - `xtensa-esp32s2-none-elf` - `xtensa-esp32s2-espidf` - `xtensa-esp32s3-none-elf` - `xtensa-esp32s3-espidf` The ESP-IDF targets already set `target_vendor="espressif"`, however, the ESP32 is, from my understanding, produced by Espressif regardless of whether using the IDF or not, so we should set the target vendor there as well?
rustc's Xtensa ESP32 targets are the following:xtensa-esp32-none-elfxtensa-esp32-espidfxtensa-esp32s2-none-elfxtensa-esp32s2-espidfxtensa-esp32s3-none-elfxtensa-esp32s3-espidfThe ESP-IDF targets already set
target_vendor="espressif", however, the ESP32 is, from my understanding, produced by Espressif regardless of whether using the IDF or not, so we should set the target vendor there as well?CC target maintainers of the affected targets: @MabezDev, @SergioGasquez
Digression: The targets are not really following the usual naming scheme, should probably have been named e.g.
xtensa_esp32s2-espressif-none-elf? Or perhaps they should be combined into one targetxtensa-espressif-none-elf, and then the user should set-Ctarget-cpu=esp32s2. But that's probably been discussed elsewhere, so I won't belabour the point.