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

Breaking change in v0.2.145 for MUSL's open64 #3264

Closed
newpavlov opened this issue Jun 5, 2023 · 5 comments
Closed

Breaking change in v0.2.145 for MUSL's open64 #3264

newpavlov opened this issue Jun 5, 2023 · 5 comments
Labels
C-bug Category: bug

Comments

@newpavlov
Copy link
Contributor

newpavlov commented Jun 5, 2023

Previously, open64 on MUSL was using the common definition with variadics. But in v0.2.145 it was migrated to a non-variadic definition. This has caused breakage of getrandom v0.1, see rust-random/getrandom#363.

cc @bossmc

@bossmc
Copy link
Contributor

bossmc commented Jun 5, 2023

Good spot, and yes indeed that's an unintended change - the whole point of that module is to back-fill a non-breaking API. Unfortunately, we can't shim variadic functions since they're unstable (as local functions) rust-lang/rust#44930. If that were merged we could write something like:

pub unsafe extern "C" fn open64(
    pathname: *const ::c_char,
    flags: ::c_int,
    mode: ...,
) -> ::c_int {
    ::open(pathname, flags, mode)
}

An earlier version of this change re-exported the LFS64 symbols with use foo as foo64 which caused two problems:

  1. The aliased symbols are the original symbols (have the same address for example) which would be a breaking change
  2. use will alias all matching symbols (which is a problem for e.g. stat64 which is a type and a function)

The latter of these doesn't matter for open/openat since there's no type to conflict with the function name, but the former would still be a concern.

@JohnTitor @Amanieu - I proposed for now we switch open/openat to be aliased with a use (with a comment to re-visit once c_variadics is stabilized. thoughts?

@bossmc
Copy link
Contributor

bossmc commented Jun 5, 2023

Oh, there's a 3rd problem with use-aliasing that also doesn't affect open/openat - if the arguments of an LFS64 function are themselves LFS64-ized then a use-aliased function won't accept them when it should. Doesn't change my previous conclusion, just providing completeness.

@Amanieu
Copy link
Member

Amanieu commented Jun 5, 2023

I agree with changing this to a use, I think this is the best way forward.

@Amanieu
Copy link
Member

Amanieu commented Jun 6, 2023

I've yanked v0.2.145 for now since it is causing widespread breakage.

SimonSapin added a commit to apollographql/router that referenced this issue Jun 6, 2023
See rust-lang/libc#3264

Requiring a yanked version causes `cargo update` to fail on CI:
https://app.circleci.com/pipelines/github/apollographql/router/12758/workflows/52d4c586-1da7-453b-b90f-1366c72ae46b/jobs/83879?invite=true#step-115-7

<!-- start metadata -->

**Checklist**

Complete the checklist (and note appropriate exceptions) before a final
PR is raised.

- [ ] Changes are compatible[^1]
- [ ] Documentation[^2] completed
- [ ] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [ ] Unit Tests
    - [ ] Integration Tests
    - [ ] Manual Tests

**Exceptions**

*Note any exceptions here*

**Notes**

[^1]. It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding
Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or
ask for it to be labeled) as `manual test`
link2xt added a commit to deltachat/deltachat-core-rust that referenced this issue Jun 6, 2023
link2xt added a commit to deltachat/deltachat-core-rust that referenced this issue Jun 6, 2023
link2xt added a commit to deltachat/deltachat-core-rust that referenced this issue Jun 6, 2023
@newpavlov
Copy link
Contributor Author

I think this issue can be closed now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: bug
Projects
None yet
Development

No branches or pull requests

4 participants