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

Fix broken build on ESP-IDF caused by #115108 #116723

Merged
merged 1 commit into from
Oct 14, 2023
Merged

Conversation

ivmarkov
Copy link
Contributor

@ijackson #115108 broke the build for ESP-IDF. I'm still checking whether this PR fixes everything - once I'm ready will remove the "Draft" status.

@dtolnay FYI

@rustbot
Copy link
Collaborator

rustbot commented Oct 14, 2023

r? @thomcc

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added O-unix Operating system: Unix-like S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Oct 14, 2023
@ivmarkov
Copy link
Contributor Author

r? @dtolnay

@rustbot rustbot assigned dtolnay and unassigned thomcc Oct 14, 2023
@rust-log-analyzer

This comment has been minimized.

@ivmarkov ivmarkov marked this pull request as ready for review October 14, 2023 11:31
Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@dtolnay
Copy link
Member

dtolnay commented Oct 14, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Oct 14, 2023

📌 Commit b3c95c5 has been approved by dtolnay

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 14, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2023
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#115439 (rustdoc: hide `#[repr(transparent)]` if it isn't part of the public ABI)
 - rust-lang#116591 (Don't accidentally detect the commit hash as an `fadd` instruction)
 - rust-lang#116603 (Reorganize `bootstrap/Cargo.toml`)
 - rust-lang#116715 (Prevent more spurious unreachable pattern lints)
 - rust-lang#116723 (Fix broken build on ESP-IDF caused by rust-lang#115108)
 - rust-lang#116730 (Add some unsoundness tests for opaques capturing hidden regions not in substs)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 456139f into rust-lang:master Oct 14, 2023
11 checks passed
@rustbot rustbot added this to the 1.75.0 milestone Oct 14, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2023
Rollup merge of rust-lang#116723 - ivmarkov:master, r=dtolnay

Fix broken build on ESP-IDF caused by rust-lang#115108

`@ijackson` rust-lang#115108 broke the build for ESP-IDF. I'm still checking whether this PR fixes everything - once I'm ready will remove the "Draft" status.

`@dtolnay` FYI
@ijackson
Copy link
Contributor

Hi. Sorry for breaking the build.

Evidently none of these are tested in CI? Obviously my local build tests weren't sufficient, anyway. My apologies.

The modules and conditionals here are getting really quite tangled and confusing. They could do with a substantial refactor. For example, this MR introduces a third almost identical copy of someone::ExitStatus::exit_ok, along with the comment.

But given the lack of CI support it's hard to see how someone doing that refactoring could avoid just making things worse. So triplicating this already-duplicated function was probably the right answer.

@ivmarkov
Copy link
Contributor Author

ivmarkov commented Oct 15, 2023

Hi. Sorry for breaking the build.

Evidently none of these are tested in CI? Obviously my local build tests weren't sufficient, anyway. My apologies.

The modules and conditionals here are getting really quite tangled and confusing. They could do with a substantial refactor. For example, this MR introduces a third almost identical copy of someone::ExitStatus::exit_ok, along with the comment.

But given the lack of CI support it's hard to see how someone doing that refactoring could avoid just making things worse. So triplicating this already-duplicated function was probably the right answer.

The ESP-IDF targets are Tier3. So no, until they stay in this tier, there is no way to test whether STD builds for those do pass via the Rust standard CI.

We catch when STD does not build for those by running our own nightly CI in the esp-rs org.

@ijackson
Copy link
Contributor

We catch when STD does not build for those by running our own nightly CI in the esp-rs org.

Ah. Thanks.

I think CI approach here could do with some refinement. I appreciate that the Rust Tier system is inteded to make it easier to get code accepted in tree by reducing the possible negative consequences for general work on Rust.

However, as an MR author, in this case I would have very much welcomed pre-merge automated feedback. Frankly, nowadays I find I rely very heavily on automated curation of codebases.

Is there a way that an MR author (or other contributor) could signal that they wish to receive a report on the correctness/status of an MR on Tier 3 platforms? If not, maybe there could be one.

The Rust policy says

In particular, do not post comments (automated or manual) on a PR that derail or suggest a block on the PR based on a tier 3 target. ... unless they have opted into such messages ...

But I don't have a way to opt in :-). (Other than maybe submitting my branch as an MR in your repo too, or something.)

I think you could run a robot which watched for @-messages, saying "please send me reports about Tier 3 targets", without violating this policy. Eg I could have written @tier3 test and received a report from your CI, and maybe others' too.

If you think this would be useful, should I file a ticket somewhere requesting it? I'm not sure where that should go. Should I propose a Rust RFC given that at least some MR authors will want one message to enable for all Tier 3 platforms?

bors-ferrocene bot added a commit to ferrocene/ferrocene that referenced this pull request Oct 17, 2023
54: Pull upstream master 2023 10 17 r=pietroalbini a=Veykril

* rust-lang/rust#116196
* rust-lang/rust#116824
* rust-lang/rust#116822
* rust-lang/rust#116477
* rust-lang/rust#116826
* rust-lang/rust#116820
  * rust-lang/rust#116811
  * rust-lang/rust#116808
  * rust-lang/rust#116805
  * rust-lang/rust#116800
  * rust-lang/rust#116798
  * rust-lang/rust#116754
* rust-lang/rust#114370
* rust-lang/rust#116804
  * rust-lang/rust#116802
  * rust-lang/rust#116790
  * rust-lang/rust#116786
  * rust-lang/rust#116709
  * rust-lang/rust#116430
  * rust-lang/rust#116257
  * rust-lang/rust#114157
* rust-lang/rust#116731
* rust-lang/rust#116550
* rust-lang/rust#114330
* rust-lang/rust#116724
* rust-lang/rust#116782
  * rust-lang/rust#116776
  * rust-lang/rust#115955
  * rust-lang/rust#115196
* rust-lang/rust#116775
* rust-lang/rust#114589
* rust-lang/rust#113747
* rust-lang/rust#116772
  * rust-lang/rust#116771
  * rust-lang/rust#116760
  * rust-lang/rust#116755
  * rust-lang/rust#116732
  * rust-lang/rust#116522
  * rust-lang/rust#116341
  * rust-lang/rust#116172
* rust-lang/rust#110604
* rust-lang/rust#110729
* rust-lang/rust#116527
* rust-lang/rust#116688
* rust-lang/rust#116757
  * rust-lang/rust#116753
  * rust-lang/rust#116748
  * rust-lang/rust#116741
  * rust-lang/rust#116594
* rust-lang/rust#116691
* rust-lang/rust#116643
* rust-lang/rust#116683
* rust-lang/rust#116635
* rust-lang/rust#115515
* rust-lang/rust#116742
  * rust-lang/rust#116661
  * rust-lang/rust#116576
  * rust-lang/rust#116540
* rust-lang/rust#116352
* rust-lang/rust#116737
  * rust-lang/rust#116730
  * rust-lang/rust#116723
  * rust-lang/rust#116715
  * rust-lang/rust#116603
  * rust-lang/rust#116591
  * rust-lang/rust#115439
* rust-lang/rust#116264
* rust-lang/rust#116727
  * rust-lang/rust#116704
  * rust-lang/rust#116696
  * rust-lang/rust#116695
  * rust-lang/rust#116644
  * rust-lang/rust#116630
* rust-lang/rust#116728
  * rust-lang/rust#116689
  * rust-lang/rust#116679
  * rust-lang/rust#116618
  * rust-lang/rust#116577
  * rust-lang/rust#115653
* rust-lang/rust#116702
* rust-lang/rust#116015
* rust-lang/rust#115822
* rust-lang/rust#116407
* rust-lang/rust#115719
* rust-lang/rust#115524
* rust-lang/rust#116705
* rust-lang/rust#116645
* rust-lang/rust#116233
* rust-lang/rust#115108
* rust-lang/rust#116670
* rust-lang/rust#116676
* rust-lang/rust#116666



Co-authored-by: Benoît du Garreau <[email protected]>
Co-authored-by: Colin Finck <[email protected]>
Co-authored-by: Ian Jackson <[email protected]>
Co-authored-by: Joshua Liebow-Feeser <[email protected]>
Co-authored-by: León Orell Valerian Liehr <[email protected]>
Co-authored-by: Trevor Gross <[email protected]>
Co-authored-by: Evan Merlock <[email protected]>
Co-authored-by: joboet <[email protected]>
Co-authored-by: Ralf Jung <[email protected]>
Co-authored-by: DaniPopes <[email protected]>
Co-authored-by: Mark Rousskov <[email protected]>
Co-authored-by: onur-ozkan <[email protected]>
Co-authored-by: Nicholas Nethercote <[email protected]>
Co-authored-by: The 8472 <[email protected]>
Co-authored-by: Samuel Thibault <[email protected]>
Co-authored-by: reez12g <[email protected]>
Co-authored-by: Jakub Beránek <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-unix Operating system: Unix-like S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants