Related Linear issue here. This is part of a set of diffs that will allow ShellCheck to be added to the CI. See inline comments for specific details of the ShellCheck error/warning output.
Details
Details
Additionally, this script is run during the Android build so if that workflow continues to pass CI we can reasonably assume that the script continues to work as expected.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
I don't have much context on this script I think, and I'm a bad choice for a first-pass reviewer except for cases listed here
Comment Actions
karol-bisztyga native/android/app/bash/detect_abis.sh: 4 line(s) 12-13, 15-16
Should probably add person who originally introduced the code as a reviewer
Comment Actions
Can you confirm that the change in line 18 doesn't change the behavior of the script before landing?
Maybe run the script locally and add some logging to make sure the outputs don't change?
| native/android/app/bash/detect_abis.sh | ||
|---|---|---|
| 18 ↗ | (On Diff #15223) | So does this change the behavior? Were we previously only getting the first element but now we're getting all elements? |
Comment Actions
Update line 12 of this diff. After reading through @ashoat and @karol's discussion on D2010 here, I realized why the regex was written like this. However, since ShellCheck complains about the right-hand side being quoted, I implemented @karol's other solution:
I could do a for loop here, but this is shorter and it works. If you think a loop would be better here, I can change it.
Making @karol a blocking reviewer so he can take a look at it since he originally committed this code.
Can you confirm that the change in line 18 doesn't change the behavior of the script before landing?
I ran the Android build locally with these changes and everything worked as expected. Although I think to properly test this script, we'd have to plug in some physical devices or something and see if it works correctly. But I have little to no experience with that, so maybe @karol can give some feedback for what a good test plan would be for this script, or if the changes made in this script should be fine.
So does this change the behavior?
Were we previously only getting the first element but now we're getting all elements?
It does change the behavior. Before, we were only printing the first element of the array. Now, we are printing all elements of the array. However, I think @karol intended to print the entire array since he put it in an echo statement, but I'm not certain.
Comment Actions
Update implementation of for loop. For some reason, the code committed was wrong and I changed it to include the variable/if check.
| native/android/app/bash/detect_abis.sh | ||
|---|---|---|
| 13 ↗ | (On Diff #15285) | ABIS was set to an empty list when this executes, I think you mean to use ${ABI} |
Comment Actions
If we have bash4+ available, you can just use associative arrays to collect results.
$ declare -A foo $ foo[a]=1 $ foo[b]=1 $ foo[a]=1 $ echo "${!foo[@]}" b a
Comment Actions
The nix environment brings in bash5, and there should be a way to bump it with homebrew https://www.shell-tips.com/mac/upgrade-bash/#gsc.tab=0
Is it worth it to add another item for people to do when getting a development environment ready? probably not. Does nix solve this issue? yes :)
| native/android/app/bash/detect_abis.sh | ||
|---|---|---|
| 13 ↗ | (On Diff #15285) | Might be mistaken, but isn't that intended? Let me walk through my thought process when writing the code. In the first iteration, ABIS is empty. So, the for loop does not execute because "${ABIS[@]}" expands to nothing (since it is empty). Then, lines 21 through 23 are run (since found is false), and "${ABI} " is appended to ABIS. Now, in future iterations, ${ABIS} is non empty and the for loop will execute. Essentially, the for loop is replacing the code that checks " ${ABI} " against every single element of ABIS. This is the old code, which uses a regex match (=~): if [[ " ${ABIS[*]} " =~ " ${ABI} " ]]; thenIn the new code, the for loop loops through each element of ABIS and checks it against "${ABI} ". If "${ABI} " is found to be equal to any element of ABIS, the loop breaks, found is set to true, and the code on line 21-23 does not execute since we have already added "${ABI} " to ABIS. |
| native/android/app/bash/detect_abis.sh | ||
|---|---|---|
| 13 ↗ | (On Diff #15285) | With all of the inline comments it was hard to see what was going on. Feel like there must be a better way to do this. However, we can revisit that later. This looks better than what was there before |
| native/android/app/bash/detect_abis.sh | ||
|---|---|---|
| 13 ↗ | (On Diff #15285) |
I agree. Associative arrays would make this so much cleaner!
Sometimes, when the diff side-by-side or unified view gets too complicated I'll either patch the diff into my codebase or just open it in a new file to be able to look at it without the inline comments. The Phabricator diff tool can be a lot. Maybe it's worth checking out some of these! https://difftastic.wilfred.me.uk/tree_diffing.html |