-
Notifications
You must be signed in to change notification settings - Fork 198
Be #1472
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
base: main
Are you sure you want to change the base?
Be #1472
Conversation
@sunfishcode Grateful for your thoughts on this. |
Seems the A few of the other commits also adress issues caused by the release of new crates which require a version of rust higher than the MSRV. Lastly, there is a commit to fix an issue whereby constants guarded by the |
# the same consistent version. We therefore need to use a version of tempfile | ||
# for which its rustix dependency is a semver match for our current version. | ||
[patch.crates-io] | ||
rustix = { path = "." } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make it impossible to do a semver incompatible release of rustix as there is no tempfile version that has a semver compatible dependency on rustix yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really see a good solution to this problem. I guess the real problem is that there is a cyclic dependency, but the two projects are both under separate ownership. The only clean solution I can see is to remove that dependency and trty to find an alternative crate. The change is outside the scope of my original PR really, I just wanted to try and get the CI working to ensure I didn't introduce any regressions. But it seems unfortunately that new releases of other crates change the result of the CI. Happy to take advice on how to proceed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One option would be to keep the big-endian CI disabled for now and only enable it once there is a new rustix release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tempfile's use of rustix is not part of rustix's testsuite. We don't need to make tempfile's version of rustix match the version of rustix using tempfile for tests. They can differ and it's fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So to be clear, I think it's best to omit this part of the patch.
@@ -286,11 +286,11 @@ pub(crate) const SIGSYS: c_int = linux_raw_sys::general::SIGSYS as _; | |||
))] | |||
pub(crate) const SIGEMT: c_int = linux_raw_sys::general::SIGEMT as _; | |||
|
|||
#[cfg(feature = "stdio")] | |||
#[cfg(any(test, feature = "stdio"))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding cfg(test)
here, please either put the test code that uses STDIN_FILENO
under cfg(feature = "stdio")
, or define STDIN_FILENO
here also when cfg(feature = "fs")
.
@bjorn3 Thanks for the feedback. Just let me know how best to proceed and I'm happy to make any changes. |
How much testing have you done with the linux_raw backend on these targets? There are some places in the Linux syscall ABI which are sensitive to endianness, such as when 32-bit values are split into two halves, so it'd be good to have some assurance that this code is working before enabling it on these targets. |
When I ran the tests locally, there were a handful of failures. But ideally I'd like to add CI for these targets, the only thing is that it requires:
I have already made most of these for some other projects, but wondered what if anything would be the best approach.
The consideration is in deciding whether to use resources from other projects or whether to replicate the same functionality to give more control. Then of course there is the additional cost in terms of resources and time for the CI. I'm happy to go away and start working on a solution if you think it would be beneficial, but would be grateful if you have some advice on which direction to take. |
rustix
to use thelinux_raw
backend onarmeb-unknown-linux-gnueabi
andaarch64_be-unknown-linux-gnu
targets. (Both of these are tier 3 rust platforms and hence no public binaries are provided).rustix
usestempfile
for its unit test, which in turn usesrustix
as a dependency we have a cyclic dependency. Change theCargo.toml
so that the version ofrustix
used to satisfy thistempfile
dependency is the working copy.