Skip to content

Conversation

@Susurrus
Copy link
Contributor

@Susurrus Susurrus commented Jun 2, 2018

This wasn't caught because the tests disable checking for termios2 on all targets, so I enabled it on musl targets to prevent this from recurring.

Closes #1006

@rust-highfive
Copy link

r? @alexcrichton

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

@alexcrichton
Copy link
Member

Has this been published? If so I think this'd be a breaking change?

If so, do we know as well how this passed ctest without being caught?

@Susurrus
Copy link
Contributor Author

Has this been published? If so I think this'd be a breaking change?

Yes it has. musl targets are all Tier-2 or worse targets, so it depends on how big of a problem it is to break things on these platforms. Though this is a bug fix. I think it'd be very useful as a contributor to have the breaking changes policy addressed in the README or CONTRIBUTOR documentation to address this.

If so, do we know as well how this passed ctest without being caught?

I'll need to dig into this a bit more. Looking at the testing, termios2 is excluded from the regular test because of import conflicts with the C headers. So it's tested as part of the second fcntl tests. termios2 is the only struct checked on non-musl platforms, but for musl targets, I'd expect that it gets checked. The relevant code is:

        if !musl {
            cfg.skip_struct(|s| {
                s != "termios2"
            });
        }

@alexcrichton
Copy link
Member

Oh dear, that sounds bad! Currently we don't ever remove APIs from libc, so perhaps these could be marked as deprecated with a message saying that they were added mistakenly and are guaranteed to not work on musl?

@Susurrus
Copy link
Contributor Author

Merged into #629 so closing this.

@Susurrus Susurrus closed this Feb 24, 2019
@Susurrus Susurrus deleted the termios2_musl branch February 24, 2019 15:55
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.

3 participants