Skip to content

Conversation

MabezDev
Copy link
Contributor

@MabezDev MabezDev commented Mar 4, 2022

Marking as draft as currently dependant on a libc fix and release.

@rust-highfive
Copy link
Contributor

r? @Mark-Simulacrum

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 4, 2022
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Collaborator

bors commented Mar 4, 2022

☔ The latest upstream changes (presumably #94612) made this pull request unmergeable. Please resolve the merge conflicts.

@Mark-Simulacrum
Copy link
Member

@rustbot author

Please mark as waiting on review (e.g., with @rustbot ready) when this is ready for review.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2022
@MabezDev MabezDev force-pushed the esp-idf-stat-type-fixes branch from 79e743a to e68b44d Compare April 7, 2022 11:56
@MabezDev MabezDev force-pushed the esp-idf-stat-type-fixes branch from e68b44d to 4f6b3a6 Compare April 11, 2022 09:51
@ivmarkov
Copy link
Contributor

LGTM - I didn't notice that off_t is imported as off64_t on non-Linux platforms.

@MabezDev MabezDev marked this pull request as ready for review April 11, 2022 11:40
@MabezDev
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 11, 2022
@gyscos
Copy link

gyscos commented Apr 13, 2022

Do we care about the horizon platform (Nintendo 3DS), which recently got similar changes to libc?
https://github.com/rust-lang/libc/pull/2714/files#diff-a828924ffbc30c564e49bc3652ffa5375a123671b228fc99b5775a29cff6eea5R33

Or, more generally, do we not want to prevent the definitions of libc (uid_t, gid_t, ...) and the ones here in std from deviating?

@ivmarkov
Copy link
Contributor

Do we care about the horizon platform (Nintendo 3DS), which recently got similar changes to libc? https://github.com/rust-lang/libc/pull/2714/files#diff-a828924ffbc30c564e49bc3652ffa5375a123671b228fc99b5775a29cff6eea5R33

Or, more generally, do we not want to prevent the definitions of libc (uid_t, gid_t, ...) and the ones here in std from deviating?

I don't think there are "std" definitions of uid_t and gid_t. STD just uses u32 (or u16 on ESP-IDF and vxworks) for uid_t and gid_t. It is another topic whether it is not better to just use u32 everywhere - even on platforms where the uid_t and gid_t are 16 bit by downcasting u32 to u16 - just like e.g. the implementation of seek - which is also fixed - and where the i64 offset is downcasted to i32 for ESP-IDF.

With all that said, there is actually no notion of a "process" on the ESP-IDF so these APIs are not supposed to be called anyway. They should just be there so that STD compiles on ESP-IDF.

@Mark-Simulacrum
Copy link
Member

Hm. I would have expected us to remove the Command APIs entirely for espidf, though historically we have not been extremely consistent on what to do for partially-supported std on various platforms.

Anyway, this PR seems ok to me.

@bors r+

@bors
Copy link
Collaborator

bors commented Apr 16, 2022

📌 Commit 4f6b3a6 has been approved by Mark-Simulacrum

@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 Apr 16, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 16, 2022
…ark-Simulacrum

espidf: fix stat

Marking as draft as currently dependant on [a libc fix](rust-lang/libc#2708) and release.
@Dylan-DPC
Copy link
Member

failed in rollup

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 16, 2022
@MabezDev MabezDev force-pushed the esp-idf-stat-type-fixes branch from 4f6b3a6 to 943a417 Compare April 16, 2022 22:29
@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Apr 16, 2022
@rust-log-analyzer

This comment has been minimized.

@MabezDev MabezDev force-pushed the esp-idf-stat-type-fixes branch 2 times, most recently from 8bb723e to 13a25d6 Compare April 19, 2022 14:21
@rust-log-analyzer

This comment has been minimized.

@MabezDev MabezDev force-pushed the esp-idf-stat-type-fixes branch from 13a25d6 to 71e22d3 Compare April 19, 2022 15:46
@rust-log-analyzer

This comment has been minimized.

* corect type usage with new type definitions in libc
@MabezDev MabezDev force-pushed the esp-idf-stat-type-fixes branch from 71e22d3 to 3569d43 Compare April 19, 2022 16:00
@MabezDev
Copy link
Contributor Author

So, off64_t isn't available on all platforms (inside libc), namely macos hence the failure in the rollup. I've added top-level imports that import off_t as off64_t using the same cfg logic that already exists for the other x64_t types.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 19, 2022
@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=iffy

@bors
Copy link
Collaborator

bors commented Apr 24, 2022

📌 Commit 3569d43 has been approved by Mark-Simulacrum

@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 Apr 24, 2022
@bors
Copy link
Collaborator

bors commented Apr 24, 2022

⌛ Testing commit 3569d43 with merge 18f314e...

@bors
Copy link
Collaborator

bors commented Apr 24, 2022

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing 18f314e to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 24, 2022
@bors bors merged commit 18f314e into rust-lang:master Apr 24, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 24, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (18f314e): comparison url.

Summary:

  • Primary benchmarks: 🎉 relevant improvement found
  • Secondary benchmarks: no relevant changes found
Regressions 😿
(primary)
Regressions 😿
(secondary)
Improvements 🎉
(primary)
Improvements 🎉
(secondary)
All 😿 🎉
(primary)
count1 0 0 1 0 1
mean2 N/A N/A -0.5% N/A -0.5%
max N/A N/A -0.5% N/A -0.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes

  2. the arithmetic mean of the percent change

N3xed added a commit to esp-rs/esp-idf-sys that referenced this pull request Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

10 participants