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

espidf: fix stat #94609

Merged
merged 1 commit into from
Apr 24, 2022
Merged

espidf: fix stat #94609

merged 1 commit into from
Apr 24, 2022

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
Collaborator

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
Contributor

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
library/std/src/sys/unix/fd.rs Outdated Show resolved Hide resolved
library/std/src/sys/unix/fd.rs Outdated Show resolved Hide resolved
library/std/src/sys/unix/fs.rs Outdated Show resolved Hide resolved
@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
Contributor

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
Contributor

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
Contributor

bors commented Apr 24, 2022

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

@bors
Copy link
Contributor

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