Fix i686 host builds #34
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the generator, we have
```rust
pub fn smallest_index(n: usize) -> usize {
for &x in [8, 16, 32].iter() {
if n < (1 << x) {
return x / 8;
}
}
64
}
```
where `x` is inferred to be `usize`. When the host architecture is
32-bits, `(1 << x)` overflows. For release build, this doesn't crash but
leads to strange invalid generated.rs which contain an undefined `u512`
type
(https://github.com/astral-sh/ruff/actions/runs/6500014395/job/17659540717). Note that the crash only happens on i686 hosts (i used the
quay.io/pypa/manylinux2014_i686 docker container), if you're cross
compiling build scripts are compiled to the host architecture and the
build works fine.
This change fixes this by instead pinning types to 4 bytes max, which
seems sufficient. I only changed this specific `usize` that unbreaks the
build, but there are more `usize` usages in the generator that might be
problematic that i haven't checked.
konstin
added a commit
to astral-sh/ruff
that referenced
this pull request
Oct 13, 2023
See progval/unicode_names2#34 for a detailed explanation. Fixes https://github.com/astral-sh/ruff/actions/runs/6500014395/job/17659540717 and should unblock astral-sh/astral-sh#44
konstin
added a commit
to astral-sh/ruff
that referenced
this pull request
Oct 13, 2023
See progval/unicode_names2#34 for a detailed explanation. Fixes https://github.com/astral-sh/ruff/actions/runs/6500014395/job/17659540717 and should unblock astral-sh/astral-sh#44
progval
reviewed
Oct 13, 2023
| } | ||
| } | ||
| 64 |
There was a problem hiding this comment.
I think it must have been, it's definitely incorrect in the old version, it should have been 8 and not 64. (I think this also explains why we saw a u512 in the broken build, we got u{64 * 8})
There was a problem hiding this comment.
Just realized we can actually know that it is unreachable: The only caller was smallest_type, which takes a Iterator<Item = u32>, and all u32 are guaranteed to be smaller than (1 << 32).
There was a problem hiding this comment.
looks like it was useful at some point... 1758fb8#diff-02c0d6c94c0983e9e9b8f82b36f950fafe18bb071f64faf825bfef437d66dbec
|
Thanks for the PR, I imagine this wasn't easy to debug |
konstin
added a commit
to astral-sh/ruff
that referenced
this pull request
Oct 13, 2023
See progval/unicode_names2#34 for a detailed explanation. Fixes https://github.com/astral-sh/ruff/actions/runs/6500014395/job/17659540717 and should unblock astral-sh/astral-sh#44
|
Pushed to crates.io as v1.2.0 (cc @chris-ha458) |
konstin
added a commit
to astral-sh/ruff
that referenced
this pull request
Oct 16, 2023
We can remove the git dependency (again). See progval/unicode_names2#34 (comment)
konstin
added a commit
to astral-sh/ruff
that referenced
this pull request
Oct 16, 2023
We can remove the git dependency (again). See progval/unicode_names2#34 (comment) I'll run the release pipeline before merging.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In the generator, we have
where
xis inferred to beusize. When the host architecture is 32-bits,(1 << x)overflows. For release build, this doesn't crash but leads to strange invalid generated.rs which contain an undefinedu512type (https://github.com/astral-sh/ruff/actions/runs/6500014395/job/17659540717). Note that the crash only happens on i686 hosts (i used the quay.io/pypa/manylinux2014_i686 docker container), if you're cross compiling build scripts are compiled to the host architecture and the build works fine.This change fixes this by instead pinning types to 4 bytes max, which seems sufficient. I only changed this specific
usizethat unbreaks the build, but there are moreusizeusages in the generator that might be problematic that i haven't checked.