Skip to content

Add mkstemp shim for unix #2346

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 3, 2022
Merged

Add mkstemp shim for unix #2346

merged 1 commit into from
Aug 3, 2022

Conversation

LegNeato
Copy link
Contributor

@LegNeato LegNeato commented Jul 8, 2022 •
edited
Loading

No description provided.

@LegNeato LegNeato force-pushed the mkstemp branch 8 times, most recently from 06e581a to e837bee Compare July 8, 2022 06:49
oli-obk
oli-obk reviewed Jul 8, 2022
View reviewed changes
@LegNeato
Copy link
Contributor Author

LegNeato commented Jul 8, 2022

Dropping this here as a reminder as it might be relevant:

https://github.com/dylni/os_str_bytes

@LegNeato
Copy link
Contributor Author

LegNeato commented Jul 8, 2022 •
edited
Loading

Duh, the target will always be unix here so all we have to do is convert at the function call boundary at runtime. I was thinking of my other realpath PR where I need to handle the complete target x host matrix. Sorry, brain is foggy. Will clean this up / remove the complexity I just added to get windows to pass.

@LegNeato
Copy link
Contributor Author

LegNeato commented Jul 9, 2022

Actually, this exists on windows, not sure how I missed it:

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/mktemp-s-wmktemp-s?view=msvc-170

I can hoist this up to a general shim, add windows tests, and clean it up.

RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 9, 2022
View reviewed changes
@RalfJung
Copy link
Member

RalfJung commented Jul 9, 2022

I can hoist this up to a general shim, add windows tests, and clean it up.

Please keep this PR to Unix targets only.
Adding this on Windows targets only makes sense after we have some more basic FS support for Windows, like opening files.

@RalfJung RalfJung added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jul 11, 2022
@LegNeato
Copy link
Contributor Author

Will get to this later in the week, thank you for the review

@LegNeato
Copy link
Contributor Author

Ok, I think all outstanding items are fixed. Thank you for your patience as I ramp up in the codebase, apologies for so much back and forth!

@bors
Copy link
Contributor

bors commented Jul 18, 2022

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

RalfJung
RalfJung reviewed Jul 19, 2022
View reviewed changes
Copy link
Member

@RalfJung RalfJung left a comment

Choose a reason for hiding this comment

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

We are getting there. :)

@RalfJung
Copy link
Member

@rustbot author
(please use @rustbot ready when it is ready for review again)

RalfJung
RalfJung reviewed Jul 19, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 19, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 19, 2022
View reviewed changes
RalfJung
RalfJung reviewed Jul 19, 2022
View reviewed changes
@LegNeato
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jul 26, 2022
RalfJung
RalfJung reviewed Aug 2, 2022
View reviewed changes
@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2022

Looks good! Just one last nit, and then please squash all the commits into one.

@bors
Copy link
Contributor

bors commented Aug 2, 2022

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

@LegNeato
Copy link
Contributor Author

LegNeato commented Aug 3, 2022

@RalfJung squashed.

@RalfJung
Copy link
Member

RalfJung commented Aug 3, 2022

Thanks. :)
@bors r+

@bors
Copy link
Contributor

bors commented Aug 3, 2022

📌 Commit b29e7b8 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Aug 3, 2022

⌛ Testing commit b29e7b8 with merge c24c638...

@bors
Copy link
Contributor

bors commented Aug 3, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing c24c638 to master...

@bors bors merged commit c24c638 into rust-lang:master Aug 3, 2022
github-actions bot pushed a commit that referenced this pull request Apr 21, 2025
@jieyouxu
Merge pull request #2346 from folkertdev/bootstrap-in-dependencies
dc98c61
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RalfJung RalfJung RalfJung left review comments

@oli-obk oli-obk oli-obk left review comments

Assignees
No one assigned
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@LegNeato @RalfJung @bors @oli-obk @rustbot