-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Fix android armeabi-v7a constraint #15248
Fix android armeabi-v7a constraint #15248
Conversation
adbc734
to
4cb7f0f
Compare
Previously this was arm instead of armv7. It seems that armv7 is the correct constraint for this case. Fixes bazelbuild#14982
4cb7f0f
to
a8680ca
Compare
Taking a look at this. This does seem like a promising fix for #14982, and is the PR I was recalling to @gregce (comment #14982 (comment)). The big question is to find out how this will work with our internal Android processes, I'm going to import this and run a lot of tests, I'll report back when that's done (it may be a few days). |
"armeabi-v7a": ["@platforms//cpu:armv7"], | ||
"armeabi-v7a": [ | ||
"@platforms//cpu:armv7", | ||
"@platforms//os:android", |
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 can't be right, shouldn't this be osx?
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.
I see, thanks for explaining.
Ran the internal tests and, as I suspected, there's no impact. I'm going to work on getting this submitted in the next few days. |
Previously this was arm instead of armv7. It seems that armv7 is the
correct constraint for this case.
Fixes #14982