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

Android: Added syscall support #3992

Merged
merged 1 commit into from
Oct 28, 2024

Conversation

YohDeadfall
Copy link
Contributor

According to the issue's description I moved the whole syscall implementation into a separate file, so it's a little bit more than just "thread APIs". Anyway, all of these system calls are available on Android.

Closes #3618.

ci/ci.sh Show resolved Hide resolved
@RalfJung
Copy link
Member

Please don't rebase when there isn't a conflict. Github is terrible at dealing with force pushes so rebasing makes re-reviewing a lot harder.

@RalfJung
Copy link
Member

RalfJung commented Oct 27, 2024

Looks good, thanks!

Please squash the commits (with git rebase --keep-base).

@RalfJung
Copy link
Member

@rustbot author

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Oct 27, 2024
@YohDeadfall
Copy link
Contributor Author

Please don't rebase when there isn't a conflict. Github is terrible at dealing with force pushes so rebasing makes re-reviewing a lot harder.

There were no conflicts at all, but for whatever reason the repository required to update the branch (can be set in the settings). This was blocking the merge ability.

Right now I see the same button, but all the checks are green and the update isn't mandatory.

@YohDeadfall
Copy link
Contributor Author

This was blocking the merge ability.

Seeing that once again, it seems that there's just an UI issue. While some checks are running, the message "This branch is out-of-date with the base branch" has a gray icon like it's mandatory while it isn't.

@YohDeadfall
Copy link
Contributor Author

@rustbot review

@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 Oct 28, 2024
@RalfJung
Copy link
Member

There were no conflicts at all, but for whatever reason the repository required to update the branch (can be set in the settings). This was blocking the merge ability.

No it doesn't block the merge, you can just ignore that message.

@RalfJung
Copy link
Member

RalfJung commented Oct 28, 2024

That force-push has a diff, so when you squashed you didn't use --keep-base... I was asking you to use that flag for a reason, it makes reviewing a lot easier. Please follow the instructions we give, they are there for a reason. (Mostly the reason is github being not great at dealing with some aspects of git, which means a lot of people need to spend a lot of time working around the deficiencies of the tool we are using.)

@RalfJung
Copy link
Member

Actually that force push diff is really strange, it goes in the wrong direction?!? What on Earth is github doing here...

@RalfJung
Copy link
Member

Or did you un-do your earlier rebase? That's not helpful, it just leads to even more diffs github can't deal with...

@RalfJung RalfJung added this pull request to the merge queue Oct 28, 2024
@RalfJung
Copy link
Member

Anyway diff still looks good, thanks for the PR!

@YohDeadfall
Copy link
Contributor Author

YohDeadfall commented Oct 28, 2024

Or did you un-do your earlier rebase? That's not helpful, it just leads to even more diffs github can't deal with...

Shouldn't be since I used --keep-base.

No it doesn't block the merge, you can just ignore that message.

Yeah, but the UI is the same as if it's mandatory. There's no way to distinguish these two cases while checks are running. That led me to thing that I had to update the branch first. It's only possible when it's finished, so the check mark is green even if there's the message.

@YohDeadfall
Copy link
Contributor Author

Buy the way, I see that this time you didn't use bors but instead the built-in merge queue. Is the Rust project switching them, or is it just that repo?

Merged via the queue into rust-lang:master with commit fb28524 Oct 28, 2024
7 checks passed
@YohDeadfall YohDeadfall deleted the android-syscall branch October 28, 2024 10:17
@RalfJung
Copy link
Member

Buy the way, I see that this time you didn't use bors but instead the built-in merge queue. Is the Rust project switching them, or is it just that repo?

The main Rust repo won't switch, but most of the rest is slowly switching.

Shouldn't be since I used --keep-base.

Very strange... but I think somehow your branch did get un-rebased. Maybe you did the rebase with the button in the github UI but didn't pull that change so when you did the squash locally and force-pushed that it undid what you did in the UI?

Yeah, but the UI is the same as if it's mandatory. There's no way to distinguish these two cases while checks are running. That led me to thing that I had to update the branch first. It's only possible when it's finished, so the check mark is green even if there's the message.

When there is an actual conflict github shows a list of files that have conflicts. It doesn't even start CI in that case. So that should be enough to distinguish the two cases.

@YohDeadfall
Copy link
Contributor Author

Very strange... but I think somehow your branch did get un-rebased. Maybe you did the rebase with the button in the github UI but didn't pull that change so when you did the squash locally and force-pushed that it undid what you did in the UI?

Indeed, just rechecked the bash history and found that I did:

git fetch origin
git reset --hard android-syscall

Note that no remote was provided, so technically it's noop. The local branch wasn't updated. Sorry for inconvenience (:

When there is an actual conflict github shows a list of files that have conflicts. It doesn't even start CI in that case. So that should be enough to distinguish the two cases.

Unfortunately not so helpful, I wrote #3998 (comment) where explained the situation. Some repositories require updates even if there are no conflicts. But if the CI isn't triggered on mandatory updates, then it might be fine. Still it requires awareness about all GH behaviors which are easy to forget or simply don't know (:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android: make std thread APIs work
3 participants