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.

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

Add fcntl OFD commands for macOS #3563

Merged
merged 1 commit into from
Aug 13, 2024
Merged

Add fcntl OFD commands for macOS #3563

merged 1 commit into from
Aug 13, 2024

Conversation

anacrolix
Copy link
Contributor

@anacrolix anacrolix commented Jan 27, 2024 •
edited
Loading

I also followed all the PR instructions and sorted the semver for apple.

@rustbot
Copy link
Collaborator

rustbot commented Jan 27, 2024

r? @JohnTitor

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

@anacrolix anacrolix mentioned this pull request Jan 28, 2024
Open
3 tasks
@anacrolix
Copy link
Contributor Author

So I noticed that the tests run on macos-13, and macos-14 is available (possibly needed for the OFD feature, despite the constants being 4 years old according to the kernel changelog), and also that the tests only run on Intel chips. I do not think it would be too much work to add aarch64 to the macos testing (assuming GitHub has a runner for this).

@JohnTitor
Copy link
Member

Could you rebase onto the latest main and squash commits into one?

@anacrolix
Copy link
Contributor Author

Yes, I'll do that soon, thanks.

@anacrolix
Copy link
Contributor Author

Okay, I've done that. It's extraordinarily painful, any reason you can't squash on merge?

@anacrolix
Copy link
Contributor Author

I don't think my change caused the macOS CI failure, I'm not sure what to do about that one.

@JohnTitor
Copy link
Member

I don't think my change caused the macOS CI failure, I'm not sure what to do about that one.

You updated the macOS version and it introduced some breaking changes, you have to account for that.

Okay, I've done that. It's extraordinarily painful, any reason you can't squash on merge?

We use the merge queue and it doesn't allow me to select the merge method sadly.

@anacrolix
Copy link
Contributor Author

It'll take someone more knowledgable about macos to fix this I think. I didn't find anything straightforward to resolve it.

@anacrolix
Copy link
Contributor Author

I found #3575 but it's mysteriously closed.

@anacrolix anacrolix mentioned this pull request Feb 11, 2024
Closed
@anacrolix anacrolix force-pushed the macos-ofd-fcntl branch 2 times, most recently from 9f5938b to cb7d5b1 Compare July 18, 2024 23:04
@anacrolix
Copy link
Contributor Author

This includes #3578 and should be merged after that.

@tgross35
Copy link
Contributor

@anacrolix please rebase and drop the commits that are no longer needed (1a8e6f4, cb7d5b1).

@rustbot author

@anacrolix
Copy link
Contributor Author

Roger

@anacrolix
Copy link
Contributor Author

@tgross35 Done

tgross35
tgross35 reviewed Aug 13, 2024
View reviewed changes
.github/workflows/full_ci.yml Outdated
on:
merge_group:
pull_request:
types: [opened, synchronize, reopened]
on: [push]
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the intent with this change? This commit should be dropped if it is only for testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this. It was frustrating having to make a PR to get CI to run. Is that a worthwhile change in another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Making a PR to get CI to run, as opposed to what exactly? Usually CI is only run on PRs and after merges.

@tgross35
Copy link
Contributor

There is also an empty change to ci/style.sh, can you drop that?

@tgross35 tgross35 added the stable-nominated This PR should be considered for cherry-pick to libc's stable release branch label Aug 13, 2024
@anacrolix
Add fcntl OFD commands for macOS
00163d2 
Obey shellcheck
ci/style.sh should be executable
Require macos-14
tgross35
tgross35 approved these changes Aug 13, 2024
View reviewed changes
Copy link
Contributor

@tgross35 tgross35 left a comment

Choose a reason for hiding this comment

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

Thanks!

@tgross35 tgross35 added this pull request to the merge queue Aug 13, 2024
Merged via the queue into rust-lang:main with commit dd8e81d Aug 13, 2024
39 checks passed
tgross35 pushed a commit to tgross35/rust-libc that referenced this pull request Aug 15, 2024
@anacrolix @tgross35
Add fcntl OFD commands for macOS
9105f6a 
Obey shellcheck
ci/style.sh should be executable
Require macos-14

(backport <rust-lang#3563>)
(cherry picked from commit 00163d2)
@tgross35 tgross35 mentioned this pull request Aug 15, 2024
Merged
@tgross35 tgross35 added stable-applied This PR has been cherry-picked to libc's stable release branch and removed stable-nominated This PR should be considered for cherry-pick to libc's stable release branch labels Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgross35 tgross35 tgross35 approved these changes

Assignees

@JohnTitor JohnTitor

Labels
S-waiting-on-author stable-applied This PR has been cherry-picked to libc's stable release branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anacrolix @rustbot @JohnTitor @tgross35