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

macOs various updates. #3578

Closed
wants to merge 2 commits into from
Closed

Conversation

devnexen
Copy link
Contributor

  • CI only for macOs arm64.
  • Fixing build issues for macOs arm64.
  • Adding macos cpu to arch api.

@rustbot
Copy link
Collaborator

rustbot commented Feb 10, 2024

r? @JohnTitor

rustbot has assigned @JohnTitor.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Cargo.toml Outdated Show resolved Hide resolved
src/unix/bsd/apple/mod.rs Outdated Show resolved Hide resolved
@anacrolix
Copy link
Contributor

This looks great! Could we get it merged?

libc-test/build.rs Outdated Show resolved Hide resolved
@JohnTitor JohnTitor added this pull request to the merge queue Feb 28, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 28, 2024
@anacrolix
Copy link
Contributor

That's an unexpected error from the merge queue.

@anacrolix
Copy link
Contributor

Woops, I think this got stuck again. Could we try to merge it again @JohnTitor @devnexen ?

@anacrolix
Copy link
Contributor

Hello again, this is ready to go! Let me know if I can help in some way?

@@ -92,5 +92,10 @@ cfg_if! {

// static_assert_eq!(core::mem::size_of::<__uint128_t>(), _SIZE_128);
// static_assert_eq!(core::mem::align_of::<__uint128_t>(), _ALIGN_128);
} else if #[cfg(all(target_arch = "aarch64", any(target_os = "macos", target_os = "ios", target_os = "tvos", target_os = "watchos")))] {
Copy link
Member

Choose a reason for hiding this comment

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

Prefer using target_vendor = "apple" instead.

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 think I ve tried already in the past and now I try again I get this

  error: `cfg(target_vendor)` is experimental and subject to change (see issue #29718)
     --> ../src/fixed_width_ints.rs:95:50
      |
  95  |       } else if #[cfg(all(target_arch = "aarch64", target_vendor = "apple"))] {
      |                                                    ^^^^^^^^^^^^^^^^^^^^^^^
      | 

Copy link
Contributor

Choose a reason for hiding this comment

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

Which job does this fail on? It looks like this should have been stable since 1.33 rust-lang/rust#57465

@Amanieu Amanieu added this pull request to the merge queue Aug 5, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 5, 2024
@Amanieu
Copy link
Member

Amanieu commented Aug 5, 2024

CI is now failing on FreeBSD 15

@tgross35 tgross35 mentioned this pull request Aug 6, 2024
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.

A couple other small changes, plus https://github.com/rust-lang/libc/pull/3578/files#r1703994194 if it works. Then I think we can merge this with the other CI fixes.

Comment on lines +2220 to +2225
// Removed in FreeBSD 15
"TDF_CANSWAP" | "TDF_SWAPINREQ" => true,

// Unaccessible in FreeBSD 15
"TDI_SWAPPED" | "P_SWAPPINGOUT" | "P_SWAPPINGIN" => true,

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind moving the freebsd-specific changes to a separate commit so cherry picking is a bit easier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you mean another PR ? usually we merge squashed commits ultimately.

Copy link
Contributor

@tgross35 tgross35 Aug 8, 2024

Choose a reason for hiding this comment

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

No, just separate commits. Usually only messy history needs to get squashed (e.g. squash the docs fix), and this specifically won't since it will get merged with other CI fixes.

No worries if it's too much trouble.

(I'm just asking because I don't know what exactly needs to get cherry picked to the 0.2 branch to get that CI working)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should be ok in theory, l ll get to it soon-ish

Comment on lines 96 to 99
/// C __int128_t (alternate name for [__int128][])
pub type __int128_t = i128;
/// C __uint128_t (alternate name for [__uint128][])
pub type __uint128_t = u128;
Copy link
Contributor

Choose a reason for hiding this comment

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

__int128 and __uint128 aren't defined in this branch of the cfg.

Suggested change
/// C __int128_t (alternate name for [__int128][])
pub type __int128_t = i128;
/// C __uint128_t (alternate name for [__uint128][])
pub type __uint128_t = u128;
/// C `__int128_t`
pub type __int128_t = i128;
/// C `__uint128_t`
pub type __uint128_t = u128;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

.github/workflows/full_ci.yml Show resolved Hide resolved
- CI only for macOs arm64.
- Fixing build issues for macOs arm64.
- Adding macos cpu to arch api.
This was referenced Aug 8, 2024
@tgross35
Copy link
Contributor

This was merged via #3797.

@tgross35 tgross35 closed this Aug 12, 2024
@anacrolix
Copy link
Contributor

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants