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

Allow name-mangling for the mbedtls_free and mbedtls_calloc functions #276

Merged
merged 2 commits into from
Jul 17, 2023

Conversation

DrTobe
Copy link
Contributor

@DrTobe DrTobe commented May 30, 2023

According to the discussion in rust-lang/rust-bindgen#1221 (comment), the \u{1} is a hint to LLVM to avoid name-mangling but this might be required on some platforms to link against the right name, e.g. on macOS.

So this fixes builds on macOS.

@jethrogb you have been part in that discussion so maybe you can evaluate if this change is the right thing to do or if this will potentially lead to other problems?

@DrTobe
Copy link
Contributor Author

DrTobe commented Jun 16, 2023

@jethrogb Could you have a look at this?

@jethrogb
Copy link
Member

I think this change is probably correct, but could you add a MacOS test in CI?

@DrTobe
Copy link
Contributor Author

DrTobe commented Jun 21, 2023

I think this change is probably correct, but could you add a MacOS test in CI?

I have no experience with Travis CI but I gave it a try. Maybe, I need some help on this, we'll see.

@DrTobe DrTobe force-pushed the allow-name-mangling branch 2 times, most recently from 2da8f9a to 620c8fe Compare June 21, 2023 21:20
@DrTobe
Copy link
Contributor Author

DrTobe commented Jun 22, 2023

@jethrogb How can I trigger Travis to pick this up?

@jethrogb
Copy link
Member

Apparently closing and re-opening may trigger a build?

@jethrogb jethrogb closed this Jun 22, 2023
@jethrogb jethrogb reopened this Jun 22, 2023
@jethrogb
Copy link
Member

Looks to be some issue with our Travis subscription, hold on...

@DrTobe
Copy link
Contributor Author

DrTobe commented Jul 10, 2023

Anything new here? As far as I can see, still no tests have run?

@Taowyoo
Copy link
Collaborator

Taowyoo commented Jul 10, 2023

Could you please rebase your PR and move the CI changes from travis CI to GitHub action workflow file at .github/workflows/test.yml. thx

@jethrogb
Copy link
Member

Haven't heard back from Travis support unfortunately, let me ping them

@DrTobe DrTobe force-pushed the allow-name-mangling branch 2 times, most recently from da35742 to 41a3d9a Compare July 11, 2023 15:29
DrTobe added 2 commits July 12, 2023 09:31
…ssl_flush_output functions

According to the discussion in
rust-lang/rust-bindgen#1221 (comment),
the \u{1} is a hint to LLVM to avoid name-mangling but this might be required
on some platforms to link against the right name, e.g. on macOS.
@DrTobe DrTobe force-pushed the allow-name-mangling branch 2 times, most recently from dca91bd to 4e005ae Compare July 12, 2023 08:09
@DrTobe
Copy link
Contributor Author

DrTobe commented Jul 12, 2023

Looks like we've got working macOS tests, yay.

Just don't make me program that yaml-shell-build.rs-cmake-chain again, please 😆

.github/workflows/test.yml Show resolved Hide resolved
@Taowyoo Taowyoo self-requested a review July 13, 2023 20:57
.github/workflows/test.yml Show resolved Hide resolved
@Taowyoo Taowyoo enabled auto-merge July 13, 2023 21:00
@Taowyoo
Copy link
Collaborator

Taowyoo commented Jul 13, 2023

Hi @jethrogb ,let's we keep track the issue of Travis CI in

auto-merge was automatically disabled July 14, 2023 07:53

Head branch was pushed to by a user without write access

@DrTobe DrTobe force-pushed the allow-name-mangling branch from 897e1a2 to 4e005ae Compare July 14, 2023 07:53
@Taowyoo Taowyoo enabled auto-merge July 17, 2023 20:34
@Taowyoo Taowyoo added this pull request to the merge queue Jul 17, 2023
Merged via the queue into fortanix:master with commit 5c18def Jul 17, 2023
@Taowyoo Taowyoo mentioned this pull request Jul 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants